Throw if module is not setup or cmd missing #64

stelcheck · 2017-08-18T09:02:57Z

Transform CommandPrefix and Commands into abstracts; this should
help provide a more comprehensive experience to newcomers
Track whether a module was setup or not, and throw an exception
when you try to call a command on a non-setup module
Throw an exception if a user command is not found

Fixes #63

stelcheck · 2017-08-18T09:03:08Z

Anything I should run for linting btw?

AlmirKadric · 2017-08-21T03:40:15Z

Mage/Module.cs

 	{
-		//
+		// Tells us if the module was set up
+		protected bool SetupCompleted = false;


is this something children classes should be able to change?
cc: @nullorvoid

No. I only wanted to make it visible.

ah, you could make a public getter with a private setter

public bool SetupCompleted { get; private set; }

Got it, will do that.

Actually, how do you set the default value on getter-setters though?

@nullorvoid is the private set implicit if no setter is defined?
refresh my memory, the initial permission is for overloading or accessing?

AlmirKadric · 2017-08-21T03:41:10Z

Mage/Module.cs

 		protected Logger Logger
 		{
 			get { return Mage.Logger(GetType().Name); }
 		}

-
-		//
+		// Static


Incomplete? Static what?
(Not saying mine was any better XD)

AlmirKadric · 2017-08-21T03:44:48Z

Mage/Module.cs

 		protected virtual List<string> StaticTopics
 		{
-			get { return null; }
+			get { return new List<string> {}; }


why an empty list, instead of null?
very definition of null in C# is empty
CC: @nullorvoid

I assume a null list would still allow you to execute list API calls, whereas null would crash if you do not check for null. Since neither the IDE nor the compiler warns against nullable types, I thought this would be safer.

AlmirKadric · 2017-08-21T03:51:48Z

Mage/Module.cs

+		// Static data setup
+		// Note that topics are not tied to MAGE modules; they are
+		// essentially global to the MAGE server instance. This is 
+		// simply a convenience function for 


looks like the sentence at the end of the paragraph is incomplete. convenience function for

AlmirKadric · 2017-08-21T04:11:28Z

Mage/Module.cs

@@ -126,17 +160,12 @@ public void SetupUsercommands(Action<Exception> cb)
 			commandHandlerActions = new Dictionary<string, Action<JObject, Action<Exception, JToken>>>();
 			commandHandlerFuncs = new Dictionary<string, Func<JObject, UserCommandStatus>>();

-			if (Commands == null)


What happens if there is a module with no usercommands?
e.g. Commands is not defined or the user does not want to define it
You're enforcing them to define it with this and the above abstract definition, but this creates code bloat inside the module. We already refactored once to remove that very bloat.
CC: @nullorvoid

You would still need to define Commands, and it would need to be an empty list.

From what I remember, In the current implementation you can go without defining that list, meaning if your module has no user commands, no need to even touch the variable. This would change that behaviour (If I'm not mistaken)

I believe this is correct. This is also why we switch from virtual to abstract and cease to provide a default definition. virtual appear to be problematic for strong type.

Honestly this comes down to how we want the user to implement it, if we force the user to override it by using abstract any modules they want will need to have the override.

The question is how many modules do NOT have commands. Also do we ever create a module without commands to start with and then add later (this is more probable than a module not having commands).

I don't honestly mind either, we can use abstract but we have to be aware when we are developing setting this up with an empty list should be ok.

AlmirKadric · 2017-08-21T04:16:26Z

Mage/Module.cs

-			get { return null; }
-		}
+		// The module name as defined on the remote MAGE server
+		protected abstract string CommandPrefix { get; }


again, the same as below you're enforcing this to be defined. IIRC the current implementation allows this to remain optional to avoid bloat in modules that have no usercommands.
CC: @nullorvoid

The only reason why you would have no user commands is in the case where you only load static data - which is not part of any official MAGE SDK, since MAGE modules do not provide topics.

yes but current mage (JS) doesn't enforce these things either
if i have no usercommands I shouldn't have to care about these values
@nullorvoid @kefniark @ronkorving comments welcome

I believe the reason why this is not enforced in the JS version is because you can create modules which are meant to be event handlers. Example: https://github.com/mage/mage-sdk-js.session/blob/master/index.js

In our case, since the current module class provides no helpers or support for defining msgStream event hooks, then its only real official purpose appears to be about making user command calls.

Same as above, for me this is a choice of how we implement, either I'm fine with.

AlmirKadric · 2017-08-21T04:22:59Z

Mage/Module.cs

 		protected virtual List<string> StaticTopics
 		{
-			get { return null; }
+			get { return new List<string> {}; }


this comment got wiped by a change, whilst its still relavent
why an empty list, instead of null?
very definition of null in C# is empty
CC: @nullorvoid

For this we have to be sure we catch all of the early returns and any that are actually continued expecting something to have been there. If there is nothing to do our methods should handle either null or empty list.

I would argue you are going to have less errors if you have empty list, but you have to make sure you check this in your functions.

- Transform CommandPrefix and Commands into abstracts; this should help provide a more comprehensive experience to newcomers - Track whether a module was setup or not, and throw an exception when you try to call a command on a non-setup module - Throw an exception if a user command is not found Fixes mage#63

stelcheck · 2017-08-22T09:04:19Z

Only issue remaining is abstract vs virtual. Let me know what the final decision is on that.

stelcheck requested review from AlmirKadric and nullorvoid August 18, 2017 09:02

AlmirKadric reviewed Aug 21, 2017

View reviewed changes

stelcheck force-pushed the fixes/missing-setup-or-command branch from a716b14 to 0851e02 Compare August 21, 2017 04:05

AlmirKadric reviewed Aug 21, 2017

View reviewed changes

stelcheck force-pushed the fixes/missing-setup-or-command branch from 0851e02 to a95ce2b Compare August 22, 2017 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw if module is not setup or cmd missing #64

Throw if module is not setup or cmd missing #64

stelcheck commented Aug 18, 2017

stelcheck commented Aug 18, 2017

AlmirKadric Aug 21, 2017

stelcheck Aug 21, 2017

AlmirKadric Aug 21, 2017 •

edited

Loading

stelcheck Aug 21, 2017

stelcheck Aug 21, 2017

AlmirKadric Aug 21, 2017 •

edited

Loading

AlmirKadric Aug 21, 2017 •

edited

Loading

AlmirKadric Aug 21, 2017

stelcheck Aug 21, 2017 •

edited

Loading

AlmirKadric Aug 21, 2017 •

edited

Loading

AlmirKadric Aug 21, 2017 •

edited

Loading

stelcheck Aug 21, 2017

AlmirKadric Aug 21, 2017

stelcheck Aug 21, 2017

nullorvoid Aug 22, 2017

AlmirKadric Aug 21, 2017

stelcheck Aug 21, 2017

AlmirKadric Aug 21, 2017

stelcheck Aug 21, 2017

nullorvoid Aug 22, 2017

AlmirKadric Aug 21, 2017

nullorvoid Aug 22, 2017

stelcheck commented Aug 22, 2017

Throw if module is not setup or cmd missing #64

Are you sure you want to change the base?

Throw if module is not setup or cmd missing #64

Conversation

stelcheck commented Aug 18, 2017

stelcheck commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlmirKadric Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlmirKadric Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

AlmirKadric Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stelcheck Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

AlmirKadric Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

AlmirKadric Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stelcheck commented Aug 22, 2017

AlmirKadric Aug 21, 2017 •

edited

Loading

AlmirKadric Aug 21, 2017 •

edited

Loading

AlmirKadric Aug 21, 2017 •

edited

Loading

stelcheck Aug 21, 2017 •

edited

Loading

AlmirKadric Aug 21, 2017 •

edited

Loading

AlmirKadric Aug 21, 2017 •

edited

Loading