Create NameArguments code fixer (15.later) #22010

jcouv · 2017-09-09T17:42:50Z

Adds a code style (Prefer_naming_literal_arguments`), an analyzer to complain about un-named literal arguments that could be named, and a fixer to rectify the situation.

Fixes #19186

@CyrusNajmabadi FYI
I plan to add support for attributes (they use different syntax) and VB, as the "AddNamedArgument" refactoring does. But I don't expect I'll be able to share much code, although I'm open to the possibility.

CyrusNajmabadi · 2017-09-09T21:15:57Z

Lol... i was right in the middle of this. Let me see your version :) #Closed

CyrusNajmabadi · 2017-09-09T21:16:45Z

src/EditorFeatures/TestUtilities/Traits.cs

@@ -104,6 +104,7 @@ public static class Features
            public const string CodeActionsUseCollectionInitializer = "CodeActions.UseCollectionInitializer";
            public const string CodeActionsUseDefaultLiteral = "CodeActions.UseDefaultLiteral";
            public const string CodeActionsUseInferredMemberName = "CodeActions.UseInferredMemberName";
+            public const string CodeActionsNameLiteralArgument = "CodeActions.UseNamedArgument";


This should be sorted properly. #Closed

CyrusNajmabadi · 2017-09-09T21:16:56Z

src/Features/CSharp/Portable/NameLiteralArgument/CSharpNameLiteralArgumentCodeFixProvider.cs

+
+namespace Microsoft.CodeAnalysis.CSharp.NameLiteralArgument
+{
+    [ExportCodeFixProvider(LanguageNames.CSharp), Shared]


Needs a name. #Resolved

CyrusNajmabadi · 2017-09-09T21:17:50Z

src/Features/CSharp/Portable/NameLiteralArgument/CSharpNameLiteralArgumentCodeFixProvider.cs

+                var argument = (ArgumentSyntax)root.FindNode(diagnostic.Location.SourceSpan);
+                var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false);
+
+                var parameterName = GetParameterName(argument, semanticModel);


it would be faster to just store hte parameter name with the diagnostic properties so we don't need to go retrieve a semantic model here. #Resolved

CyrusNajmabadi · 2017-09-09T21:18:40Z

src/Features/CSharp/Portable/NameLiteralArgument/CSharpNameLiteralArgumentCodeFixProvider.cs

+                var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false);
+
+                var parameterName = GetParameterName(argument, semanticModel);
+                var newArgument = SyntaxFactory.Argument(SyntaxFactory.NameColon(parameterName), default, argument.Expression);


you could just do argument.WithNameColon. Also, you should probably remove teh trivia from the original argument and add it to the new argument. #Resolved

CyrusNajmabadi · 2017-09-09T21:19:05Z

src/Features/CSharp/Portable/NameLiteralArgument/CSharpNameLiteralArgumentCodeFixProvider.cs

+            }
+
+            int index = arguments.IndexOf(argument);
+            var parameters = semanticModel.GetSymbolInfo(argument.Parent.Parent).Symbol.GetParameters();


this is assuming a lot. #Resolved

CyrusNajmabadi · 2017-09-09T21:19:38Z

src/Features/CSharp/Portable/NameLiteralArgument/CSharpNameLiteralArgumentDiagnosticAnalyzer.cs

+        public override bool OpenFileOnly(Workspace workspace)
+            => false;
+
+        protected override void InitializeWorker(AnalysisContext context)


why not just register for SyntaxKind.Argument? #Closed

This seemed cheaper (only check the options and get the symbol once per invocation). #Closed

ok, that's somewhat reasonable :) #Closed

CyrusNajmabadi · 2017-09-09T21:19:56Z

src/Features/CSharp/Portable/NameLiteralArgument/CSharpNameLiteralArgumentDiagnosticAnalyzer.cs

+            {
+                return;
+            }
+            if (!optionSet.GetOption(CSharpCodeStyleOptions.PreferNamingLiteralArguments).Value)


newline. #Resolved

CyrusNajmabadi · 2017-09-09T21:20:47Z

src/Features/CSharp/Portable/NameLiteralArgument/CSharpNameLiteralArgumentDiagnosticAnalyzer.cs

+
+            var node = context.Node;
+            SeparatedSyntaxList<ArgumentSyntax> arguments;
+            switch (node)


Duplicated code with fix provider. Either make extension, or make into a static helper here that both can use. #Resolved

CyrusNajmabadi · 2017-09-09T21:21:03Z

src/Features/CSharp/Portable/NameLiteralArgument/CSharpNameLiteralArgumentDiagnosticAnalyzer.cs

+                return;
+            }
+
+            var symbolInfo = context.SemanticModel.GetSymbolInfo(context.Node);


you have a local for context.Node below. use that. #Resolved

insteafd of storing into symbolInfo. Just do a .Symbol here and store into 'var symbol'. #Resolved

CyrusNajmabadi · 2017-09-09T21:21:59Z

src/Features/CSharp/Portable/NameLiteralArgument/CSharpNameLiteralArgumentDiagnosticAnalyzer.cs

+                    continue;
+                }
+
+                if (parameters[i].IsParams)


Slightly concerned about error cases (though that shouldn't probably happen if .Symbol was non-null). But what if there are less parameters than arguments? #Resolved

Nit: combine all three checks into one. #Resolved

CyrusNajmabadi · 2017-09-09T21:22:09Z

src/Features/CSharp/Portable/NameLiteralArgument/CSharpNameLiteralArgumentDiagnosticAnalyzer.cs

+            }
+        }
+
+        private bool IsPositionalArgument(ArgumentSyntax argument)


nit: use => #Resolved

nit: doesn't seem useful enough to have it's own method. #Resolved

CyrusNajmabadi · 2017-09-09T21:24:00Z

src/VisualStudio/Core/Def/ServicesVSResources.resx

-    <value>Prefere local function over anonymous function</value>
+    <value>Prefer local function over anonymous function</value>
+  </data>
+  <data name="Prefer_naming_literal_arguments" xml:space="preserve">


i feel like we might want to put in the name (C# 7.2 and above). The reason here is that the feature could make sense even in C# 7.1 and below, but we're choosing not to offer it because we don't want to then name all the subsequent parameters.

I think the option should be: "Prefer named literal arguments (C# 7.2 and greater)"

CyrusNajmabadi · 2017-09-09T21:25:42Z

But I don't expect I'll be able to share much code,

You shoudl be able to share a ton of code here between C# and VB. #Closed

CyrusNajmabadi · 2017-09-09T21:27:43Z

Two medium issues for me.

I feel like we might want this to be more flexible. i.e. a "Use named arguments" feature that starts with the options "always, never, for_literals". We've had requests from some people that they always want named args (i.e. objective-c style).
We now have a refactoring and analyzer that are overlapping. i.e. the AddNamedArgument refactoring will conflict with your fix provider. So users will see two options on a literal to add a named argument. We need to unify these so that users always get the option to add the name if possible, but also that based on their options they can see squiggles/fix-all.

CyrusNajmabadi · 2017-09-09T21:28:20Z

src/EditorFeatures/CSharpTest/NameLiteralArgument/NameLiteralArgumentTests.cs

+    {
+        M(a, /*before*/ b: 2 /*after*/, /*before*/ c: 3 /*after*/);
+    }
+}", parseOptions: s_parseOptions);


need multiline tests. #Resolved

CyrusNajmabadi · 2017-09-09T21:30:18Z

src/Features/CSharp/Portable/NameLiteralArgument/CSharpNameLiteralArgumentCodeFixProvider.cs

+
+                var parameterName = GetParameterName(argument, semanticModel);
+                var newArgument = SyntaxFactory.Argument(SyntaxFactory.NameColon(parameterName), default, argument.Expression);
+                editor.ReplaceNode(argument, newArgument);


If this cahnges to a feature where users can add named args in more places (i.e. 'always' or 'for_lambdas') then this won't be right. Consider:

Foo(Bar(5))

there are two named arguments that can be added to make: Foo(arg: Bar(val: 5)). To support this, you need to call the .ReplaceNode overload that takes a lambda and gives you the current node, so that you see the changes of nested updates.

CyrusNajmabadi · 2017-09-09T21:31:18Z

src/Features/CSharp/Portable/NameLiteralArgument/CSharpNameLiteralArgumentDiagnosticAnalyzer.cs

+    {
+        public CSharpNameLiteralArgumentDiagnosticAnalyzer()
+            : base(IDEDiagnosticIds.NameLiteralArgumentDiagnosticId,
+                   new LocalizableResourceString(nameof(FeaturesResources.Name_literal_argument), FeaturesResources.ResourceManager, typeof(FeaturesResources)))


should have two resource strings. One should be something like "Prefer named literal argument". It is what will go in the error list. #Resolved

This is still needed. #Resolved

CyrusNajmabadi · 2017-09-09T21:32:02Z

src/Features/CSharp/Portable/NameLiteralArgument/CSharpNameLiteralArgumentDiagnosticAnalyzer.cs

+                return;
+            }
+
+            var parameters = symbolInfo.Symbol.GetParameters();


then you won't ahve to do symbolInfo.Symbol here. #Resolved

CyrusNajmabadi · 2017-09-09T21:32:35Z

src/Features/CSharp/Portable/NameLiteralArgument/CSharpNameLiteralArgumentDiagnosticAnalyzer.cs

+                    continue;
+                }
+
+                context.ReportDiagnostic(


because you have the parameter here, you can just grab the name and add it to the Properties object for the diagnostic. This will save expensive work on the code fix provider side. #Resolved

CyrusNajmabadi · 2017-09-09T21:33:50Z

src/Workspaces/CSharp/Portable/CodeStyle/CSharpCodeStyleOptions.cs

@@ -154,6 +154,12 @@ internal static class CSharpCodeStyleOptions
                EditorConfigStorageLocation.ForBoolCodeStyleOption("csharp_prefer_inferred_anonymous_type_member_names"),
                new RoamingProfileStorageLocation($"TextEditor.CSharp.Specific.{nameof(PreferInferredAnonymousTypeMemberNames)}")});

+        public static readonly Option<CodeStyleOption<bool>> PreferNamingLiteralArguments = new Option<CodeStyleOption<bool>>(
+            nameof(CodeStyleOptions), nameof(PreferNamingLiteralArguments), defaultValue: CodeStyleOptions.TrueWithSuggestionEnforcement,


this should not be on by default. Or it should be true with none enforcement. We don't want tons of ...'s showing up in people's code. #Resolved

CyrusNajmabadi · 2017-09-09T21:35:05Z

src/Workspaces/CSharp/Portable/CodeStyle/CSharpCodeStyleOptions.cs

+        public static readonly Option<CodeStyleOption<bool>> PreferNamingLiteralArguments = new Option<CodeStyleOption<bool>>(
+            nameof(CodeStyleOptions), nameof(PreferNamingLiteralArguments), defaultValue: CodeStyleOptions.TrueWithSuggestionEnforcement,
+            storageLocations: new OptionStorageLocation[] {
+                EditorConfigStorageLocation.ForBoolCodeStyleOption("csharp_prefer_naming_literal_arguments"),


dotnet_style_prefer_named_literal_arguments (since this will be supported for Vb right? i thought you said VB had non-trailing arg support).

That said, i think i would prefer that this was:

dotnet_style_prefer_named_arguments=always,never,for_literals,for_... (i.e. for_lambdas, for_etc).

This leaves us open for extending this in the future. bools make that harder. #Resolved

If supported for VB, should move to CodeStyleOptions #Resolved

CyrusNajmabadi · 2017-09-09T21:41:26Z

src/Features/CSharp/Portable/NameLiteralArgument/CSharpNameLiteralArgumentDiagnosticAnalyzer.cs

+                    continue;
+                }
+
+                if (!argument.Expression.IsAnyLiteralExpression())


Note: this extension needs to be updated to handle 'default' literals. (please add test as well).

Also, consider removing extension entirely. I'm not sure i see a purpose in it given that you can write "is LiteralExpressionSyntax". While kind checks are faster than typechecks, this has to check like 8 kinds. At that point I doubt it is any faster. #Resolved

CyrusNajmabadi · 2017-09-09T21:47:09Z

Done with pass. #Closed

No VB implementation yet

jcouv · 2017-09-11T17:02:14Z

📝 I'm still working on the code style view model, to be able to display the choices.

jcouv · 2017-09-11T17:16:48Z

src/VisualStudio/CSharp/Impl/Options/Formatting/StyleViewModel.cs

@@ -793,6 +810,7 @@ internal StyleViewModel(OptionSet optionSet, IServiceProvider serviceProvider) :
            CodeStyleItems.Add(new BooleanCodeStyleOptionViewModel(CSharpCodeStyleOptions.PreferInferredTupleNames, ServicesVSResources.Prefer_inferred_tuple_names, s_preferInferredTupleName, s_preferInferredTupleName, this, optionSet, expressionPreferencesGroupTitle));
            CodeStyleItems.Add(new BooleanCodeStyleOptionViewModel(CSharpCodeStyleOptions.PreferInferredAnonymousTypeMemberNames, ServicesVSResources.Prefer_inferred_anonymous_type_member_names, s_preferInferredAnonymousTypeMemberName, s_preferInferredAnonymousTypeMemberName, this, optionSet, expressionPreferencesGroupTitle));
            CodeStyleItems.Add(new BooleanCodeStyleOptionViewModel(CSharpCodeStyleOptions.PreferLocalOverAnonymousFunction, ServicesVSResources.Prefer_local_function_over_anonymous_function, s_preferLocalFunctionOverAnonymousFunction, s_preferLocalFunctionOverAnonymousFunction, this, optionSet, expressionPreferencesGroupTitle));
+            CodeStyleItems.Add(new BooleanCodeStyleOptionViewModel(CodeStyleOptions.PreferNamedArguments, ServicesVSResources.Prefer_named_literal_arguments_CSharp_7_2_or_greater, s_preferNamedArguments, s_preferNamedArguments, this, optionSet, expressionPreferencesGroupTitle));


@CyrusNajmabadi
I discussed how to structure the options with DavidP.
We're thinking it may be better to stick with boolean options (rather than enum options), but have more than one (one for literals, one for lambdas, one for everything) when we want, under one section.
Then it would look like the section for "var" preferences:

In the meantime, it would be a single boolean option for literals.

This would also solve a technical problem. When I tried to fix the code above (since BooleanCodeStyleOptionViewModel because I pass an option that's wrong), the constructor for EnumCodeStyleOptionViewModel does not seem to handle per-language options, but only plain options.

Let me know if that approach seems ok.

CyrusNajmabadi · 2017-09-11T18:18:27Z

Let me know if that approach seems ok.

I defer all decisions to @dpoeschl :)

That said, while it would absolutely be expedient to represent this as booleans for our current impl, we may want to consider if that really makes sense for the future.

I think we should design the editor config options to make the most sense in isolation (which, to me, means allowing "prefer_named_arguments=for_literals,for_anonymous_functions", and then figure out a good UI for this. For example, i believe resharper suports this through drop down lists that have checkboxes in them.

NOte that the UI can come in the future. since you're not currently doing "for_anonymous_functions" we wouldn't have to worry about how to make the UI work here.

CyrusNajmabadi · 2017-09-11T18:19:10Z

Note: this is hte problem i mentioned earlier:

We def do not want both of these options shown to the user. We likely need to merge the existing refactoring into the analyzer you're creating.

jcouv · 2017-09-12T17:38:30Z

@CyrusNajmabadi Thanks for the clarification.
Here's what I could do in terms of merging the new codefixer and the existing refactoring: I add a preference option "always" which produces a diagnostic on every argument that could be named. That should reproduce the behavior of the existing refactoring.

I'm assuming that the desired behavior on FixAll on a literal argument is to let the user choose what they want to do: (1) name all literal arguments (in document/projects/solution), or (2) name all arguments (also in various scopes).
Do we have an example of code fixer with such FixAll behavior that I could look at?

dpoeschl · 2017-09-13T18:23:05Z

Paging @sharwell who usually knows of exotic FixAlls or otherwise has thoughts on them.

jcouv · 2017-09-13T22:18:19Z

From discussion with @heejaechang, @rchande and @kuhlenh:
We could use nested fixes (like suppression or add using), but they are not common and they are rather awkward to navigate.
We could have a custom dialog (like "Change Signature", but that is also non-standard and seems overkill.
That leaves us with top-level fixes.

From discussion with Kasey, if we have three code style preferences (literals, lambdas, all), the latter one should be off by default to avoid spamming the lightbulb when invoked on a line, and to play nice with some future "apply all code styles" capability.
The first two could be on, but with no severity.

Then we have to decide what to do about the existing "Add argument name" refactoring. Kasey will see if it is used. (1) We could just remove it. Or (2) we could make it conditional (don't trigger if the diagnostic from the code analyzer/fixer is there, assuming that the refactoring has access to that information).

CyrusNajmabadi · 2017-09-13T22:40:12Z

From discussion with Kasey, if we have three code style preferences (literals, lambdas, all), the latter one should be off by default to avoid spamming the lightbulb when invoked on a line

I'm not sure if that's a problem. i mean the refactoring already always offers this. So it's no worse than today.

To me, this entire feature makes much more sense just as a single analyzer. i.e. we remove the existing refactoring and do everything as one analyzer. The analyzer always runs and always allows you to trigger the "add named arg" feature if in a valid location. All that the options control is if we show things up as squiggles/dots/error-list messages (depending on the location and waht option you picked).

jcouv · 2017-09-13T22:54:20Z

I think that's workable. The only downside of removing the refactoring is that we lose the ability to add argument names including trailing names.

There is still the question of behavior for FixAll. Would the following make sense?

If you click on a diagnostic on a literal and invoke FixAll, we'll fix all literal arguments.
If you click on a diagnostic on a lambda and invoke FixAll, we'll fix all lambda arguments.
If you click on a diagnostic on an argument that is not a literal or lambda, and invoke FixAll, we'll fix all arguments (including literals and lambdas).

CyrusNajmabadi · 2017-09-13T23:02:50Z

The only downside of removing the refactoring is that we lose the ability to add argument names including trailing names.

Why?

The analyzer would still just mark the nodes in the question. The codefixprovider for that analyzer could offerboth fixes.

CyrusNajmabadi · 2017-09-13T23:04:17Z

If you click on a diagnostic on a literal and invoke FixAll, we'll fix all literal arguments.

We have flexibility here. Probably what makes most sense is to have FixAll behavior offered and contingent upon your settings. So if you have "Prefer named arguments for XXX = true" and you fix-all then we fix up all the things that match your preference.

By default you would have no preference, and thus fix-all woudl not be offered. Once you indicate your preferences, then you get fixall

sharwell · 2017-09-13T23:15:51Z

I'm primarily concerned about the limitations of the current option. In particular, we're providing a Fix All that's going to get some "colorful" feedback regarding the changes to this code:

var list = new List<int>();
list.Add(0);
list.Add(1);

As for the limitation to literals:

I believe this limitation makes sense for the first implementation, but I can imagine numerous scenarios where we may want to expand it. For example, 2+2 is not a literal, but users may want to have it named anyway. At this point, I would be in favor of continuing to restrict the implementation to focusing on literals, but change the presentation to just mention "named arguments", etc.

As for the code fix/refactoring:

A code fix should be provided to fix warnings
The code fix should support Fix All operations, which corrects only locations (arguments) which the analyzers would report according the current code style
A refactoring should be provided to fix the argument in the current caret context
The refactoring should not be offered if the current caret context would be reported as a diagnostic

Future work

This feature should be defined in a way that it can be expanded to support an option to remove unnecessary arguments. This may tie into the same preference regarding the use of named literals, or it could use a new option. The default value for the setting would be suggesting the removal of explicit argument names in cases where the rules for tuple element name inference would have inferred the same name.

sharwell · 2017-09-13T23:18:48Z

@jcouv When you get this working, if at all possible please create a branch in your Roslyn fork and execute the Fix All demonstrating the impact of the default behavior on this repository. You may need to temporarily remove the C# 7.2 limitation in the analysis to get it to pass but it should allow you to create the branch.

jcouv · 2017-09-14T02:12:40Z

@sharwell Thanks for the feedback. Last commit removes "literal" from the presentation and setting name.

I'm now working on the refactoring, so that it doesn't trigger when the analyzer already reports a diagnostic.
When I call GetDiagnostics() on an argument syntax, from within the refactoring code, I don't get the diagnostic introduced by the analyzer. Is that expected?

jcouv · 2017-09-29T22:31:41Z

From discussion with Jason, we should not have a code fixer and a refactoring implementing similar logic. I'm going to try and convert the existing refactoring into a fixer, and then have the analyzer produce diagnostics for literal, but also produce diagnostics for any argument that could be named.

@mavasani Jason recommend that I validate this design with you. Adding diagnostics for every argument that could be named seems like a lot... I'll stop by to discuss. Thanks

jcouv · 2018-01-03T23:30:07Z

Closed because I found out that in practice the existing refactoring is sufficient. Also, my experiment with running such a fixer on the Roslyn codebase was actually rather annoying (consider all the Assert methods, with literals for the expected values).

Create NameLiteralArgument code fixer (C#)

2758a9b

jcouv added the Area-Compilers label Sep 9, 2017

jcouv added this to the 15.5 milestone Sep 9, 2017

jcouv self-assigned this Sep 9, 2017

dnfclas added the cla-already-signed label Sep 9, 2017

CyrusNajmabadi reviewed Sep 9, 2017

View reviewed changes

Address PR feedback from Cyrus

cda97f3

No VB implementation yet

jcouv commented Sep 11, 2017

View reviewed changes

jcouv changed the title ~~Create NameLiteralArgument code fixer (C#)~~ Create NameArguments code fixer Sep 12, 2017

Revert to boolean option and add VB codestyle choice

1fe2655

jcouv force-pushed the non-trailing-fix branch from a586378 to 1fe2655 Compare September 12, 2017 04:07

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Sep 12, 2017

Merge remote-tracking branch 'dotnet/master' into non-trailing-fix

7429afd

Avoid term 'literal' in presentation and setting name

b6005fa

jcouv changed the title ~~Create NameArguments code fixer~~ Create NameArguments code fixer (15.later) Oct 11, 2017

jcouv modified the milestones: 15.5, 15.later Oct 11, 2017

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Oct 19, 2017

jcouv changed the base branch from master to post-dev15.5-contrib October 19, 2017 18:07

shyamnamboodiripad changed the base branch from post-dev15.5-contrib to master November 17, 2017 20:16

jcouv closed this Jan 3, 2018

mavasani mentioned this pull request Sep 16, 2020

Improvement for Display inline parameter name hints #47597

Closed

Create NameArguments code fixer (15.later) #22010

Create NameArguments code fixer (15.later) #22010

Conversation

jcouv commented Sep 9, 2017 • edited Loading

CyrusNajmabadi commented Sep 9, 2017 • edited by jcouv Loading

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Sep 9, 2017 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017

Choose a reason for hiding this comment

CyrusNajmabadi commented Sep 9, 2017 • edited by jcouv Loading

CyrusNajmabadi commented Sep 9, 2017

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 10, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi Sep 9, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi commented Sep 9, 2017 • edited by jcouv Loading

jcouv commented Sep 11, 2017

jcouv Sep 11, 2017

Choose a reason for hiding this comment

CyrusNajmabadi commented Sep 11, 2017

CyrusNajmabadi commented Sep 11, 2017

jcouv commented Sep 12, 2017

dpoeschl commented Sep 13, 2017

jcouv commented Sep 13, 2017

CyrusNajmabadi commented Sep 13, 2017

jcouv commented Sep 13, 2017

CyrusNajmabadi commented Sep 13, 2017

CyrusNajmabadi commented Sep 13, 2017

sharwell commented Sep 13, 2017

As for the limitation to literals:

As for the code fix/refactoring:

Future work

jcouv commented Sep 9, 2017 •

edited

Loading

CyrusNajmabadi commented Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

jcouv Sep 9, 2017 •

edited

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi commented Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 10, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi Sep 9, 2017 •

edited by jcouv

Loading

CyrusNajmabadi commented Sep 9, 2017 •

edited by jcouv

Loading