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

Value forms for name are less special, can be overridden #867

Merged

Conversation

seancpeters
Copy link
Contributor

Primary changes:

  1. The default name is setup as a symbol in SimpleConfigModel.FromJObject(), and used unless explicitly overridden from template configuration. If the template config provides name symbol configuration, without value forms, the default name forms are used. But if the explicitly defined name symbol defines values forms, those are used exclusively for name.
  2. Direct ParameterSymbol replacements are now setup via the identity value form (internal to SimpleConfigModel).
  3. Parameters with value forms explicitly defined in the template are not given an identity replacement. This is a break from past behavior, in which a direct replacement was setup, in addition to any explicitly defined value forms.

Ancillary changes:

  1. Fixed a bug in TrieEvaluator.cs so that if multiple terminals exist as matches to the same pattern, the effective terminal is the last-in, instead of the first in.
  2. Modified a unit test to deal with the fact that parameters with value forms explicitly defined no longer get an identity replacement.
  3. Modified TemplateCreator.InstantiateAsync so that separate IParameterSets are used for GetCreationEffects() and CreateAsync(). Using the same set was causing duplication of macros produced.

@mlorbetske
Copy link
Contributor

@dotnet-bot test CentOS7.1 x64 Debug Build please
@dotnet-bot test OSX10.12 x64 Release Build please

@mlorbetske mlorbetske changed the base branch from rel/vs2017/3-Preview2 to rel/vs2017/3-Preview3 June 16, 2017 07:24
@mlorbetske
Copy link
Contributor

Is this good to go? We haven't talked about this one in a while. I seem to recall that there's a behavior change in here, let's talk through it again tomorrow - at this point, I think we'd want to keep the behavior with what's currently out there the same.

@seancpeters
Copy link
Contributor Author

seancpeters commented Jun 22, 2017

The second commit deals with the issues of breaking behavior from the first commit:

  1. Trie creation will now filter duplicated match patterns, with a last in wins behavior. This does the same thing as filtering at match time, but is a slight performance increase since the filtering only needs to occur once.
  2. All symbols with value forms defined (including name) get the "identity" value form prepended to their list. This can be overridden with the "addIdentity": false configuration option when using the extended value form configuration format.

Notable change:
The syntax of value form configuration in template.json now has 2 separate syntaxes:

Note: in the examples below, "global" is used. But we'll be extending this to allow
conditional forms, which will have other names.
The same format will be used for other named form definitions.

Simple:
"forms": {
   "global": [ <strings representing value form names> ]
}

Detailed:
"forms" {
   "global": {
     "forms": [ <strings representing value form names> ],
     "addIdentity": <bool default true>,
     // other future extensions, e.g. conditionals
   },

If the symbol doesn't include an "identity" form and the addIdentity flag isn't false,
an identity value form is added to the beginning of the symbol's value form list.
If there is an identity form listed, its position remains intact irrespective of the addIdentity flag.

This setup allows for backwards compatibility with existing configuration processing prior to this change.

@@ -925,7 +925,12 @@ private static ISymbolModel SetupDefaultNameSymbol(string sourceName)
""description"": ""The default name symbol"",
""datatype"": ""string"",
""forms"": {
""global"": [ ""identity"", ""safe_name"", ""lower_safe_name"", ""safe_namespace"", ""lower_safe_namespace""]
""global"": [ """ + IdentityValueForm.FormName
Copy link
Contributor

Choose a reason for hiding this comment

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

I think interpolation would be cleaner here, the brace doubling everywhere else might make it not be though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went back & forth on how to best format this too. Offline we decided to keep as is.

throw new InvalidCastException("templateDefinedName is not a ParameterSymbol");
}

if (!(defaultDefinedName is ParameterSymbol defaultSymbol))
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is the only place defaultDefinedName is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but what it's cast to is used later in the method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, didn't see the symbol after the is, oops

""description"": ""The default name symbol"",
""datatype"": ""string"",
""forms"": {
""global"": [ """ + IdentityValueForm.FormName
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible good place for interpolation

@mlorbetske
Copy link
Contributor

Should we make the fix for #381 as part of this or in a subsequent PR?

@seancpeters
Copy link
Contributor Author

I think it'd be best to work on #381 in a separate PR, so this doesn't get too big.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants