Complete command builders implementation

In theory this should just work, more testing is needed though

src/Discord.Net.Commands/Builders/CommandBuilder.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Linq;
 using System.Threading.Tasks;
 using System.Collections.Generic;
 
@@ -22,6 +23,8 @@ namespace Discord.Commands.Builders
         public string Name { get; set; }
         public string Summary { get; set; }
         public string Remarks { get; set; }
+        public RunMode RunMode { get; set; }
+        public int Priority { get; set; }
         public Func<CommandContext, object[], IDependencyMap, Task> Callback { get; set; }
         public ModuleBuilder Module { get; }
 
@@ -47,6 +50,18 @@ namespace Discord.Commands.Builders
             return this;
         }
 
+        public CommandBuilder SetRunMode(RunMode runMode)
+        {
+            RunMode = runMode;
+            return this;
+        }
+
+        public CommandBuilder SetPriority(int priority)
+        {
+            Priority = priority;
+            return this;
+        }
+
         public CommandBuilder SetCallback(Func<CommandContext, object[], IDependencyMap, Task> callback)
         {
             Callback = callback;
@@ -75,6 +90,28 @@ namespace Discord.Commands.Builders
 
         internal CommandInfo Build(ModuleInfo info, CommandService service)
         {
+            if (aliases.Count == 0)
+                throw new InvalidOperationException("Commands require at least one alias to be registered");
+
+            if (Callback == null)
+                throw new InvalidOperationException("Commands require a callback to be built");
+
+            if (Name == null)
+                Name = aliases[0];
+
+            if (parameters.Count > 0)
+            {
+                var lastParam = parameters[parameters.Count - 1];
+
+                var firstMultipleParam = parameters.FirstOrDefault(x => x.Multiple);
+                if ((firstMultipleParam != null) && (firstMultipleParam != lastParam))
+                    throw new InvalidOperationException("Only the last parameter in a command may have the Multiple flag.");
+                
+                var firstRemainderParam = parameters.FirstOrDefault(x => x.Remainder);
+                if ((firstRemainderParam != null) && (firstRemainderParam != lastParam))
+                    throw new InvalidOperationException("Only the last parameter in a command may have the Remainder flag.");
+            }
+
             return new CommandInfo(this, info, service);
         }
     }

src/Discord.Net.Commands/Builders/ModuleBuilder.cs

@@ -83,6 +83,15 @@ namespace Discord.Commands.Builders
 
         public ModuleInfo Build(CommandService service)
         {
+            if (aliases.Count == 0)
+                throw new InvalidOperationException("Modules require at least one alias to be registered");
+
+            if (commands.Count == 0 && submodules.Count == 0)
+                throw new InvalidOperationException("Tried to build empty module");
+
+            if (Name == null)
+                Name = aliases[0];
+
             return new ModuleInfo(this, service);
         }
     }

src/Discord.Net.Commands/Builders/ParameterBuilder.cs

@@ -81,9 +81,19 @@ namespace Discord.Commands.Builders
 
         internal ParameterInfo Build(CommandInfo info, CommandService service)
         {
+            // TODO: should we throw when we don't have a name?
+            if (Name == null)
+                Name = "[unknown parameter]";
+
+            if (ParameterType == null)
+                throw new InvalidOperationException($"Could not build parameter {Name} from command {info.Name} - An invalid parameter type was given");
+
             if (TypeReader == null)
                 TypeReader = service.GetTypeReader(ParameterType);
 
+            if (TypeReader == null)
+                throw new InvalidOperationException($"Could not build parameter {Name} from command {info.Name} - A valid TypeReader could not be found");
+
             return new ParameterInfo(this, info, service);
         }
     }

src/Discord.Net.Commands/CommandService.cs

@@ -7,22 +7,26 @@ using System.Reflection;
 using System.Threading;
 using System.Threading.Tasks;
 
+using Discord.Commands.Builders;
+
 namespace Discord.Commands
 {
     public class CommandService
     {
         private readonly SemaphoreSlim _moduleLock;
-        private readonly ConcurrentDictionary<Type, ModuleInfo> _moduleDefs; 
+        private readonly ConcurrentDictionary<Type, ModuleInfo> _typedModuleDefs; 
         private readonly ConcurrentDictionary<Type, TypeReader> _typeReaders;
+        private readonly ConcurrentBag<ModuleInfo> _moduleDefs;
         private readonly CommandMap _map;
 
-        public IEnumerable<ModuleInfo> Modules => _moduleDefs.Select(x => x.Value);
-        public IEnumerable<CommandInfo> Commands => _moduleDefs.SelectMany(x => x.Value.Commands);
+        public IEnumerable<ModuleInfo> Modules => _typedModuleDefs.Select(x => x.Value);
+        public IEnumerable<CommandInfo> Commands => _typedModuleDefs.SelectMany(x => x.Value.Commands);
 
         public CommandService()
         {
             _moduleLock = new SemaphoreSlim(1, 1);
-            _moduleDefs = new ConcurrentDictionary<Type, ModuleInfo>();
+            _typedModuleDefs = new ConcurrentDictionary<Type, ModuleInfo>();
+            _moduleDefs = new ConcurrentBag<ModuleInfo>();
             _map = new CommandMap();
             _typeReaders = new ConcurrentDictionary<Type, TypeReader>
             {
@@ -63,6 +67,22 @@ namespace Discord.Commands
         }
 
         //Modules
+        public async Task<ModuleInfo> BuildModule(Action<ModuleBuilder> buildFunc)
+        {
+            await _moduleLock.WaitAsync().ConfigureAwait(false);
+            try
+            {
+                var builder = new ModuleBuilder();
+                buildFunc(builder);
+
+                var module = builder.Build(this);
+                return LoadModuleInternal(module);
+            }
+            finally
+            {
+                _moduleLock.Release();
+            }
+        }
         public async Task<ModuleInfo> AddModule<T>()
         {
             await _moduleLock.WaitAsync().ConfigureAwait(false);
@@ -70,17 +90,17 @@ namespace Discord.Commands
             {
                 var typeInfo = typeof(T).GetTypeInfo();
 
-                if (_moduleDefs.ContainsKey(typeof(T)))
+                if (_typedModuleDefs.ContainsKey(typeof(T)))
                     throw new ArgumentException($"This module has already been added.");
 
-                var module = ModuleClassBuilder.Build(this, typeInfo).First();
+                var module = ModuleClassBuilder.Build(this, typeInfo).FirstOrDefault();
 
-                _moduleDefs[typeof(T)] = module;
+                if (module.Value == default(ModuleInfo))
+                    throw new InvalidOperationException($"Could not build the module {typeof(T).FullName}, did you pass an invalid type?");
 
-                foreach (var cmd in module.Commands)
-                    _map.AddCommand(cmd);
-
-                return module;
+                _typedModuleDefs[module.Key] = module.Value;
+                
+                return LoadModuleInternal(module.Value);
             }
             finally
             {
@@ -89,29 +109,44 @@ namespace Discord.Commands
         }
         public async Task<IEnumerable<ModuleInfo>> AddModules(Assembly assembly)
         {
-            var moduleDefs = ImmutableArray.CreateBuilder<ModuleInfo>();
             await _moduleLock.WaitAsync().ConfigureAwait(false);
             try
             {
-                var types = ModuleClassBuilder.Search(assembly);
-                return ModuleClassBuilder.Build(types, this).ToImmutableArray();
+                var types = ModuleClassBuilder.Search(assembly).ToArray();
+                var moduleDefs = ModuleClassBuilder.Build(types, this);
+
+                foreach (var info in moduleDefs)
+                {
+                    _typedModuleDefs[info.Key] = info.Value;
+                    LoadModuleInternal(info.Value);
+                }
+
+                return moduleDefs.Select(x => x.Value).ToImmutableArray();
             }
             finally
             {
                 _moduleLock.Release();
             }
         }
+        private ModuleInfo LoadModuleInternal(ModuleInfo module)
+        {
+            _moduleDefs.Add(module);
+
+            foreach (var command in module.Commands)
+                _map.AddCommand(command);
+
+            foreach (var submodule in module.Submodules)
+                LoadModuleInternal(submodule);
+            
+            return module;
+        }
 
         public async Task<bool> RemoveModule(ModuleInfo module)
         {
             await _moduleLock.WaitAsync().ConfigureAwait(false);
             try
             {
-                var type = _moduleDefs.FirstOrDefault(x => x.Value == module);
-                if (default(KeyValuePair<Type, ModuleInfo>).Key == type.Key)
-                    throw new KeyNotFoundException($"Could not find the key for the module {module?.Name ?? module?.Aliases?.FirstOrDefault()}");
-
-                return RemoveModuleInternal(type.Key);
+                return RemoveModuleInternal(module);
             }
             finally
             {
@@ -123,24 +158,33 @@ namespace Discord.Commands
             await _moduleLock.WaitAsync().ConfigureAwait(false);
             try
             {
-                return RemoveModuleInternal(typeof(T));
+                ModuleInfo module;
+                _typedModuleDefs.TryGetValue(typeof(T), out module);
+                if (module == default(ModuleInfo))
+                    return false;
+                
+                return RemoveModuleInternal(module);
             }
             finally
             {
                 _moduleLock.Release();
             }
         }
-        private bool RemoveModuleInternal(Type type)
+        private bool RemoveModuleInternal(ModuleInfo module)
         {
-            ModuleInfo unloadedModule;
-            if (_moduleDefs.TryRemove(type, out unloadedModule))
+            var defsRemove = module;
+            if (!_moduleDefs.TryTake(out defsRemove))
+                return false;
+            
+            foreach (var cmd in module.Commands)
+                _map.RemoveCommand(cmd);
+
+            foreach (var submodule in module.Submodules)
             {
-                foreach (var cmd in unloadedModule.Commands)
-                    _map.RemoveCommand(cmd);
-                return true;
+                RemoveModuleInternal(submodule);
             }
-            else
-                return false;
+
+            return true;
         }
 
         //Type Readers

src/Discord.Net.Commands/Info/CommandInfo.cs

@@ -37,10 +37,14 @@ namespace Discord.Commands
             Summary = builder.Summary;
             Remarks = builder.Remarks;
 
+            RunMode = builder.RunMode;
+            Priority = builder.Priority;
+
             Aliases = module.Aliases.Permutate(builder.Aliases, (first, second) => first + " " + second).ToImmutableArray();
             Preconditions = builder.Preconditions.ToImmutableArray();
 
             Parameters = builder.Parameters.Select(x => x.Build(this, service)).ToImmutableArray();
+            HasVarArgs = builder.Parameters.Count > 0 ? builder.Parameters[builder.Parameters.Count - 1].Multiple : false;
 
             _action = builder.Callback;
         }

src/Discord.Net.Commands/Info/ModuleInfo.cs

@@ -17,6 +17,7 @@ namespace Discord.Commands
         public IReadOnlyList<string> Aliases { get; }
         public IEnumerable<CommandInfo> Commands { get; }
         public IReadOnlyList<PreconditionAttribute> Preconditions { get; }
+        public IReadOnlyList<ModuleInfo> Submodules { get; }
 
         internal ModuleInfo(ModuleBuilder builder, CommandService service)
         {
@@ -29,6 +30,8 @@ namespace Discord.Commands
             Aliases = BuildAliases(builder).ToImmutableArray();
             Commands = builder.Commands.Select(x => x.Build(this, service));
             Preconditions = BuildPreconditions(builder).ToImmutableArray();
+
+            Submodules = BuildSubmodules(builder, service).ToImmutableArray();
         }
 
         private static IEnumerable<string> BuildAliases(ModuleBuilder builder)
@@ -59,13 +62,24 @@ namespace Discord.Commands
             return result;
         }
 
+        private static List<ModuleInfo> BuildSubmodules(ModuleBuilder parent, CommandService service)
+        {
+            var result = new List<ModuleInfo>();
+
+            foreach (var submodule in parent.Modules)
+            {
+                result.Add(submodule.Build(service));
+            }
+
+            return result;
+        }
+
         private static List<PreconditionAttribute> BuildPreconditions(ModuleBuilder builder)
         {
             var result = new List<PreconditionAttribute>();
 
-
             ModuleBuilder parent = builder;
-            while (parent.ParentModule != null)
+            while (parent != null)
             {
                 result.AddRange(parent.Preconditions);
                 parent = parent.ParentModule;

src/Discord.Net.Commands/Utilities/ModuleClassBuilder.cs

@@ -25,8 +25,8 @@ namespace Discord.Commands
             }
         }
 
-        public static IEnumerable<ModuleInfo> Build(CommandService service, params TypeInfo[] validTypes) => Build(validTypes, service);
-        public static IEnumerable<ModuleInfo> Build(IEnumerable<TypeInfo> validTypes, CommandService service)
+        public static Dictionary<Type, ModuleInfo> Build(CommandService service, params TypeInfo[] validTypes) => Build(validTypes, service);
+        public static Dictionary<Type, ModuleInfo> Build(IEnumerable<TypeInfo> validTypes, CommandService service)
         {
             if (!validTypes.Any())
                 throw new InvalidOperationException("Could not find any valid modules from the given selection");
@@ -36,22 +36,20 @@ namespace Discord.Commands
 
             var builtTypes = new List<TypeInfo>();
 
-            var result = new List<ModuleInfo>();
+            var result = new Dictionary<Type, ModuleInfo>();
 
             foreach (var typeInfo in topLevelGroups)
             {
                 // this shouldn't be the case; may be safe to remove?
-                if (builtTypes.Contains(typeInfo))
+                if (result.ContainsKey(typeInfo.AsType()))
                     continue;
 
-                builtTypes.Add(typeInfo);
-
                 var module = new ModuleBuilder();
 
                 BuildModule(module, typeInfo, service);
                 BuildSubTypes(module, typeInfo.DeclaredNestedTypes, builtTypes, service);
 
-                result.Add(module.Build(service));
+                result[typeInfo.AsType()] = module.Build(service);
             }
 
             return result;
@@ -61,15 +59,18 @@ namespace Discord.Commands
         {
             foreach (var typeInfo in subTypes)
             {
+                if (!IsValidModuleDefinition(typeInfo))
+                    continue;
+                
                 if (builtTypes.Contains(typeInfo))
                     continue;
-
-                builtTypes.Add(typeInfo);
                 
                 builder.AddSubmodule((module) => {
                     BuildModule(module, typeInfo, service);
                     BuildSubTypes(module, typeInfo.DeclaredNestedTypes, builtTypes, service);
                 });
+
+                builtTypes.Add(typeInfo);
             }
         }
 
@@ -89,7 +90,10 @@ namespace Discord.Commands
                 else if (attribute is AliasAttribute)
                     builder.AddAliases((attribute as AliasAttribute).Aliases);
                 else if (attribute is GroupAttribute)
+                {
+                    builder.Name = builder.Name ?? (attribute as GroupAttribute).Prefix;
                     builder.AddAliases((attribute as GroupAttribute).Prefix);
+                }
                 else if (attribute is PreconditionAttribute)
                     builder.AddPrecondition(attribute as PreconditionAttribute);
             }
@@ -111,8 +115,17 @@ namespace Discord.Commands
             foreach (var attribute in attributes)
             {
                 // TODO: C#7 type switch
-                if (attribute is NameAttribute)
+                if (attribute is CommandAttribute)
+                {
+                    var cmdAttr = attribute as CommandAttribute;
+                    builder.AddAliases(cmdAttr.Text);
+                    builder.RunMode = cmdAttr.RunMode;
+                    builder.Name = builder.Name ?? cmdAttr.Text;
+                }
+                else if (attribute is NameAttribute)
                     builder.Name = (attribute as NameAttribute).Text;
+                else if (attribute is PriorityAttribute)
+                    builder.Priority = (attribute as PriorityAttribute).Priority;
                 else if (attribute is SummaryAttribute)
                     builder.Summary = (attribute as SummaryAttribute).Text;
                 else if (attribute is RemarksAttribute)
@@ -154,15 +167,15 @@ namespace Discord.Commands
             var attributes = paramInfo.GetCustomAttributes();
             var paramType = paramInfo.ParameterType;
 
+            builder.Name = paramInfo.Name;
+
             builder.Optional = paramInfo.IsOptional;
             builder.DefaultValue = paramInfo.HasDefaultValue ? paramInfo.DefaultValue : null;
 
             foreach (var attribute in attributes)
             {
                 // TODO: C#7 type switch
-                if (attribute is NameAttribute)
-                    builder.Name = (attribute as NameAttribute).Text;
-                else if (attribute is SummaryAttribute)
+                if (attribute is SummaryAttribute)
                     builder.Summary = (attribute as SummaryAttribute).Text;
                 else if (attribute is ParamArrayAttribute)
                 {
@@ -193,6 +206,7 @@ namespace Discord.Commands
                 }
             }
 
+            builder.ParameterType = paramType;
             builder.TypeReader = reader;
         }
 
@@ -205,7 +219,7 @@ namespace Discord.Commands
         private static bool IsValidCommandDefinition(MethodInfo methodInfo)
         {
             return methodInfo.IsDefined(typeof(CommandAttribute)) &&
-                   methodInfo.ReturnType != typeof(Task) &&
+                   methodInfo.ReturnType == typeof(Task) &&
                    !methodInfo.IsStatic &&
                    !methodInfo.IsGenericMethod;
         }