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

Make all identifiers for enumSourceValue in EnumMappings as single-quoted References #2736

Closed
wants to merge 1 commit into from

Conversation

sprisha
Copy link
Contributor

@sprisha sprisha commented Mar 27, 2024

For Enum Mappings, if a user passes enum references as a source value using form Mode like this:
image
it converts to text mode like this:
image
And this fails compiling because the latter is unable to parse properly with the existing antlr rules.

So now, we will make it so we single-quote each identifier in a enum reference, like this:
image
This compiles properly and fixes the roundtrip issue.

What type of PR is this?

Bug fix for studio

What does this PR do / why is it needed ?

Described above.

Which issue(s) this PR fixes:

Fixes #

Other notes for reviewers:

Does this PR introduce a user-facing change?

No.

@sprisha sprisha requested a review from a team as a code owner March 27, 2024 16:38
@sprisha sprisha changed the title Make all identifiers for enumSourceValue in EnumMappings as single-qu… Make all identifiers for enumSourceValue in EnumMappings as single-quoted References Mar 27, 2024
@@ -150,7 +150,7 @@ public static String convertIdentifier(String val, boolean doubleQuotes)
}
else
{
return UNQUOTED_IDENTIFIER_PATTERN.matcher(val).matches() ? val : convertString(val, true, doubleQuotes);
return convertString(val, true, doubleQuotes);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a good change, as will trigger many breaks on test cases, and will impact users that dont need quote.

I though the change is either to enhance the regex, or enhance the grammar definition so we can parse the value properly.

Copy link

Test Results

     70 files   -      683       70 suites   - 683   3m 46s ⏱️ - 1h 2m 14s
1 631 tests  - 10 684  1 466 ✔️  - 10 684  4 💤  - 161  161 +161 
2 343 runs   - 13 019  2 178 ✔️  - 13 009  4 💤  - 171  161 +161 

For more details on these failures, see this check.

Results for commit bc48593. ± Comparison against base commit ea6829a.

@finos-admin
Copy link
Member

This PR is stale because it has been open for 30 days with no activity. Please remove stale label or add any comment to keep this open. Otherwise this will be closed in 5 days.

@finos-admin
Copy link
Member

This PR was closed because it has been inactive for 35 days. Please re-open if this PR is still relevant.

@finos-admin finos-admin closed this May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants