Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for parsing multiple types of quotation marks in commands, Fix #942 #943

Merged
merged 23 commits into from
May 25, 2018
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
605630f
Add ability to support different types of quotation marks
Chris-Johnston Jan 28, 2018
2290a62
merge conflicts to update with latest branch
Chris-Johnston Jan 28, 2018
384b0ca
Added normal quotation mark to list of aliases, removed single quote …
Chris-Johnston Jan 28, 2018
4f6e29b
clean up leftover changes from testing
Chris-Johnston Jan 28, 2018
3e54e6f
change quotation mark parsing to use a map of matching pairs
Chris-Johnston Jan 29, 2018
2e27d26
remove commented out code
Chris-Johnston Jan 29, 2018
6633a5c
Fix conventions of the command parser utility functions
Chris-Johnston Jan 29, 2018
d42936c
change storage type of alias dictionary to be IReadOnlyDictionary
Chris-Johnston Jan 29, 2018
46e9cf6
revert type of CommandServiceConfig QuotationMarkAliasMap to Dictionary
Chris-Johnston Jan 29, 2018
9c58d0e
minor formatting changes to CommandParser
Chris-Johnston Jan 29, 2018
23ba23b
remove unnecessary whitespace
Chris-Johnston Jan 29, 2018
1a8aa96
Move aliases outside of CommandInfo class
Chris-Johnston Jan 29, 2018
f0fe5d6
copy IReadOnlyDictionary to ImmutableDictionary
Chris-Johnston Jan 29, 2018
8187f35
minor syntax changes in CommandServiceConfig
Chris-Johnston Jan 29, 2018
befb65e
add newline before namespace for consistency
Chris-Johnston Jan 30, 2018
2cb6750
newline formatting tweak
Chris-Johnston Feb 1, 2018
7fbd0ab
simplification of GetMatch method for CommandParser
Chris-Johnston Feb 3, 2018
ac394c2
add more quote unicode punctuation pairs
Chris-Johnston Feb 3, 2018
12d5a70
add check for null value when building ImmutableDictionary
Chris-Johnston Feb 3, 2018
fd3533f
Move default alias map into a separate source file
Chris-Johnston Feb 5, 2018
89ae95b
Merge branch 'dev' into quotationMarks
Chris-Johnston Mar 27, 2018
0704eab
Merge branch 'dev' into quotationMarks
Chris-Johnston May 12, 2018
820f9e3
Ensure that the collection passed into command service is not null
Chris-Johnston May 12, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions src/Discord.Net.Commands/CommandParser.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Text;
using System.Threading.Tasks;
Expand All @@ -13,7 +14,7 @@ private enum ParserPart
Parameter,
QuotedParameter
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove unnecessary whitespace

public static async Task<ParseResult> ParseArgsAsync(CommandInfo command, ICommandContext context, bool ignoreExtraArgs, IServiceProvider services, string input, int startPos)
{
ParameterInfo curParam = null;
Expand All @@ -24,7 +25,32 @@ public static async Task<ParseResult> ParseArgsAsync(CommandInfo command, IComma
var argList = ImmutableArray.CreateBuilder<TypeReaderResult>();
var paramList = ImmutableArray.CreateBuilder<TypeReaderResult>();
bool isEscaping = false;
char c;
char c, matchQuote = '\0';

// local helper functions
bool IsOpenQuote(IReadOnlyDictionary<char, char> dict, char ch)
{
// return if the key is contained in the dictionary if it exists
if (dict != null)
return dict.ContainsKey(ch);
// or otherwise if it is the default double quote
return c == '\"';
}

char GetMatch(IReadOnlyDictionary<char, char> dict, char ch)
{
// get the corresponding value for the key, if it exists
if (dict != null)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could just be reduced down to (dict != null && dict.TryGetValue(c, out var value)).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AntiTcb How about (dict?.TryGetValue(c ,out var value) ?? false)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just make sure that the collection that gets passed in is always initialized so that it's at least empty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of assuming that the collection is always initialized. Added in 820f9e3

{
char value;
if (dict.TryGetValue(c, out value))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out var value would probably preferable here

{
return value;
}
}
// or get the default pair of the default double quote
return '\"';
}

for (int curPos = startPos; curPos <= endPos; curPos++)
{
Expand Down Expand Up @@ -74,9 +100,11 @@ public static async Task<ParseResult> ParseArgsAsync(CommandInfo command, IComma
argBuilder.Append(c);
continue;
}
if (c == '\"')

if(IsOpenQuote(command._quotationAliases, c))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing whitespace after if

{
curPart = ParserPart.QuotedParameter;
matchQuote = GetMatch(command._quotationAliases, c);
continue;
}
curPart = ParserPart.Parameter;
Expand All @@ -97,7 +125,7 @@ public static async Task<ParseResult> ParseArgsAsync(CommandInfo command, IComma
}
else if (curPart == ParserPart.QuotedParameter)
{
if (c == '\"')
if(c == matchQuote)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing whitespace after if

{
argString = argBuilder.ToString(); //Remove quotes
lastArgEndPos = curPos + 1;
Expand Down
3 changes: 2 additions & 1 deletion src/Discord.Net.Commands/CommandService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ public class CommandService
internal readonly RunMode _defaultRunMode;
internal readonly Logger _cmdLogger;
internal readonly LogManager _logManager;
internal readonly IReadOnlyDictionary<char, char> _quotationMarkAliasMap;

public IEnumerable<ModuleInfo> Modules => _moduleDefs.Select(x => x);
public IEnumerable<CommandInfo> Commands => _moduleDefs.SelectMany(x => x.Commands);
Expand All @@ -45,6 +46,7 @@ public CommandService(CommandServiceConfig config)
_ignoreExtraArgs = config.IgnoreExtraArgs;
_separatorChar = config.SeparatorChar;
_defaultRunMode = config.DefaultRunMode;
_quotationMarkAliasMap = config.QuotationMarkAliasMap;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make an ImmutableDictionary out of it here?

Well, I guess the consideration is: do we want to guarantee immutable semantics internally, or fake it and take a perf win but also allow the map to be mutated while the client is running (which isn't too hard to do while implemented this way)? cc: @foxbot @FiniteReality

Either way, I'd make the storage field an IReadOnlyDictionary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For completion's sake: There is a third option where we can fake the immutable semantics, but make it much harder for users to modify the map after initialization. (Still wouldn't completely prevent it, though.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, I've left this as an IReadOnlyDictionary. I found that when the dictionary could be modified by the user externally it didn't break anything, but I could understand that being a poor design decision.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't see your second comment. I think I ended up doing your third option. I've been able to test it like this:

CommandServiceConfig config = new CommandServiceConfig();
var d = new Dictionary<char, char>()
{
    { '-', '-' },
    {'\"', '\"' }
};
config.QuotationMarkAliasMap = d;
commands = new CommandService(config);
d.Add('~', '~');

Again, I've found that it still works but might not be the intended idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, that's the first thing that I talked about. The third option would be to do

_quotationMarkAliasMap = new Dictionary(config.QuotationMarkAliasMap);

Also, the type on the CommandServiceConfig didn't need to be changed to IReadOnlyDictionary. This now prohibits doing the following:

new CommandServiceConfig()
{
    QuotationMarkAliasMap =
    {
        { '[', ']' } //for example
    }
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But anyway, I'd like to hear fox and finite's inputs. Do we want the internals to be semantically correct, or not take a perf hit?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say make an ImmutableDictionary out of it, because that's what we do in other places .

if (_defaultRunMode == RunMode.Default)
throw new InvalidOperationException("The default run mode cannot be set to Default.");

Expand Down Expand Up @@ -277,7 +279,6 @@ public Task<IResult> ExecuteAsync(ICommandContext context, int argPos, IServiceP
public async Task<IResult> ExecuteAsync(ICommandContext context, string input, IServiceProvider services = null, MultiMatchHandling multiMatchHandling = MultiMatchHandling.Exception)
{
services = services ?? EmptyServiceProvider.Instance;

var searchResult = Search(context, input);
if (!searchResult.IsSuccess)
return searchResult;
Expand Down
27 changes: 26 additions & 1 deletion src/Discord.Net.Commands/CommandServiceConfig.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
namespace Discord.Commands
using System.Collections.Generic;
namespace Discord.Commands
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newline between these for consistency

{
public class CommandServiceConfig
{
Expand All @@ -16,6 +17,30 @@ public class CommandServiceConfig
/// <summary> Determines whether RunMode.Sync commands should push exceptions up to the caller. </summary>
public bool ThrowOnError { get; set; } = true;

/// <summary> Collection of aliases that can wrap strings for command parsing.
/// represents the opening quotation mark and the value is the corresponding closing mark.</summary>
public Dictionary<char, char> QuotationMarkAliasMap { get; set; }
= new Dictionary<char, char>()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant parentheses here. Do we need the type name too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type name is required until dotnet/csharplang#100 lands. Do we omit the parentheses in other places too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure we omit the parentheses where possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub doesn't seem to show this as outdated correctly, but please see commit 8187f35

{
{'\"', '\"' },
{'«', '»' },
{'‘', '’' },
{'“', '”' },
{'„', '‟' },
{'‹', '›' },
{'‚', '‛' },
{'《', '》' },
{'〈', '〉' },
{'「', '」' },
{'『', '』' },
{'〝', '〞' },
{'﹁', '﹂' },
{'﹃', '﹄' },
{'"', '"' },
{''', ''' },
{'「', '」' }
};

/// <summary> Determines whether extra parameters should be ignored. </summary>
public bool IgnoreExtraArgs { get; set; } = false;
}
Expand Down
4 changes: 3 additions & 1 deletion src/Discord.Net.Commands/Info/CommandInfo.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using Discord.Commands.Builders;
using Discord.Commands.Builders;
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
Expand All @@ -20,6 +20,7 @@ public class CommandInfo

private readonly CommandService _commandService;
private readonly Func<ICommandContext, object[], IServiceProvider, CommandInfo, Task> _action;
internal readonly IReadOnlyDictionary<char,char> _quotationAliases;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this should not be stored on the command, but I'm not sure if we have the ability to pass the command service to the parser where ParseArgsAsync is called. Might be worth taking a look?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at this again, and found that I could use the similar behavior of IgnoreExtraArgs.

https://github.com/Chris-Johnston/Discord.Net/blob/46e9cf64f3d3635de872720c308e922b13ab8114/src/Discord.Net.Commands/Info/CommandInfo.cs#L124


public ModuleInfo Module { get; }
public string Name { get; }
Expand Down Expand Up @@ -65,6 +66,7 @@ internal CommandInfo(CommandBuilder builder, ModuleInfo module, CommandService s
HasVarArgs = builder.Parameters.Count > 0 ? builder.Parameters[builder.Parameters.Count - 1].IsMultiple : false;

_action = builder.Callback;
_quotationAliases = service._quotationMarkAliasMap;
_commandService = service;
}

Expand Down