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

fix #4650 - adding validation for multichoice symbols #4698

Merged

Conversation

JanKrivanek
Copy link
Member

Problem

#4650
Choice symbols with multiple values allowed parse some strings (default values, choice value sent over API etc.) and split them on | or , chars - hence if those chars would be present in the actual choice values, results would be broken.

Solution

Adding template validation step, that validates choice symbols marked as allowing multiple values. Those choice symbols cannot have any choices that would contain the magic split chars

Checks:

  • Added unit tests
  • Added #nullable enable to all the modified files ?

Comment on lines 128 to 131
<data name="Authoring_InvalidMultichoiceSymbol" xml:space="preserve">
<value>Choice parameter {0} is invalid. It allows multiple values ('AllowMultipleValues=true'), while some of the configured choices contain separator characters. Invalid choices: {1}</value>
<comment>{0} is the offending symbol name, {1} is a csv list of offending choice values</comment>
</data>
Copy link
Member

Choose a reason for hiding this comment

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

I like the wording here! It clearly describes the choice parameter that failed validation and why. One suggestion - I think we should include the valid separator character(s) in the message.

Suggested change
<data name="Authoring_InvalidMultichoiceSymbol" xml:space="preserve">
<value>Choice parameter {0} is invalid. It allows multiple values ('AllowMultipleValues=true'), while some of the configured choices contain separator characters. Invalid choices: {1}</value>
<comment>{0} is the offending symbol name, {1} is a csv list of offending choice values</comment>
</data>
<data name="Authoring_InvalidMultichoiceSymbol" xml:space="preserve">
<value>Choice parameter {0} is invalid. It allows multiple values ('AllowMultipleValues=true'), while some of the configured choices contain the separator character(s) {1}. Invalid choices: {2}</value>
<comment>{0} is the offending symbol name, {1} is a set of separator characters, and {2} is a csv list of offending choice values</comment>
</data>

Copy link
Member Author

Choose a reason for hiding this comment

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

Good improvement @baronfel! Incorporating.

Copy link
Member

@baronfel baronfel left a comment

Choose a reason for hiding this comment

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

Minor feedback on the error message, but it looks very clear to me. Great work 👍

@JanKrivanek JanKrivanek merged commit 7466482 into dotnet:feature/multichoices May 10, 2022
@JanKrivanek JanKrivanek deleted the multichoice-validation branch May 10, 2022 12:30
JanKrivanek added a commit that referenced this pull request May 13, 2022
* #4490 implementing support for quote-less choice literals (#4574)

* #4490 implementing support for quote-less choice literals

* Testfix

* Make the unquoted literals feature opt-in via EnableQuotelessLiterals property

* Localize error message

* Fix code and tests after merge

* Multichoice implementation contd (#4666)

* #4490 implementing support for quote-less choice literals

* #4490 multichoice parameters implementation

* Add Tab completion test cases

* Fix code + tests after merge

* Fix code and unit tests after merging

* Add support for multichoice parameters in join macro

* Improve based on review comments

* fix #4650 - adding validation for multichoice symbols (#4698)

* fix #4650 - adding validation for multichoice symbols

* Adjust error message

* Multichoice refactor quoteless (#4708)

* #4665 Refactoring part 1: Make EnableQuotelessLiterals exposed only to Orchestrator

* Fix parameters localization

* Fix after merge
JanKrivanek added a commit that referenced this pull request May 26, 2022
…ft.TemplateEngine.Abstractions (#4717)

* #4490 implementing support for quote-less choice literals (#4574)

* #4490 implementing support for quote-less choice literals

* Testfix

* Make the unquoted literals feature opt-in via EnableQuotelessLiterals property

* Localize error message

* Fix code and tests after merge

* Multichoice implementation contd (#4666)

* #4490 implementing support for quote-less choice literals

* #4490 multichoice parameters implementation

* Add Tab completion test cases

* Fix code + tests after merge

* Fix code and unit tests after merging

* Add support for multichoice parameters in join macro

* Improve based on review comments

* fix #4650 - adding validation for multichoice symbols (#4698)

* fix #4650 - adding validation for multichoice symbols

* Adjust error message

* #4665 Refactoring part 1: Make EnableQuotelessLiterals exposed only to Orchestrator

* Fix parameters localization

* #4665: Refactor step 2 - lower the Core dependency on Abstractions

* Fix tests - proper fixtures typing

* Fix tests after refactorings - mainly issues with PhysicalFileSystem

* Fix tests

* Revert back to mounted file system in orchestrator for source files

* fix api

* Improve - further removal of IEnvironemnt, test logger improvement, nullability to api, other

* Fix test bug - add log providers to test log factory
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