-
Notifications
You must be signed in to change notification settings - Fork 320
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
Replace Guava's ImmutableMap in XtextAntlrGeneratorFragment2 #2990
base: main
Are you sure you want to change the base?
Replace Guava's ImmutableMap in XtextAntlrGeneratorFragment2 #2990
Conversation
�IF partitions.size > 1� | ||
�FOR partition : partitions.indexed� | ||
private static final class Init�partition.key� { | ||
private static void doInit(�ImmutableMap�.Builder<�AbstractElement�, �String�> builder, �grammar.grammarAccess� grammarAccess) { | ||
private static void doInit(�Map�<�AbstractElement�, �String�> builder, �grammar.grammarAccess� grammarAccess) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call the parameter map
now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it as it is since the passed map is still some kind of a 'builder' since it is just used to build the final immutable copy. And it required less charges.
But if you prefer just map
, I'll change that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HannesWell I see your point, but builder
sounds strange to me; let's wait for @szarnekow 's review anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understand. What about naming it according to its content? So just mappings
?
In general repeating the type in the variable name is often superfluous from a Clean-Code point of view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mappings
sounds fine to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@szarnekow are you fine as well with mappings
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that’s a good fit
To be sure it doesn't break anything, shouldn't we regenerate all the languages with this change? |
yes but this is tedious work as many workflows dont work and we need to fiddle a lot with mwe deps to get them running |
Actually, I fixed the workflows when I regenerated the test languages. In any case, what I meant is that we must make sure this PR doesn't break the semantics |
@LorenzoBettini the workflows work but won’t start cause mwe launch and maybe others missing |
@cdietrich which ones? The one of test languages works because I'm using it for other branches. Also the one of common types works IIRC. In that case it's a java launch, not a mwe2 launch. Same for xbase and xtend, they're java applications. |
@LorenzoBettini at least the following are not working org.eclipse.xtext.builder.standalone.tests/src/org/eclipse/xtext/builder/tests/GenerateBuilderTestLanguages.mwe2 (Am not sure if i managed to run all) |
@HannesWell now that #2998 is fixed, you should be able to re-generate the languages and see if the build is still green. Maybe, however, it's better to wait for @szarnekow 's review before doing the re-generation of the languages. |
Thanks for fixing that. So should I simply import all languages into a secondard Eclipse that already contains this change and just re-run all workflows once we are fine with the change itself? |
@HannesWell, there's no need to do that in a secondary Eclipse: the mwe2 workflow uses the classes from the current workspace's classpath, where you already have the new fragments. |
a126d43
to
e5f5b1c
Compare
Since we all agreed on the new variable name above and there where no other objectsion I rerun all worksflows and added the re-generated Parser implementations in a second commit to this PR. Please let me know if there is anything that should be changed. |
In the failing test |
Alternatively the HashMap could only be wrapped into a |
That would be my preference |
All the changes are internal respectively just affects the internals of generated code.
Part of #2975