-
Notifications
You must be signed in to change notification settings - Fork 18
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
Migrate MWE (almost entirely) off Guava #287
base: master
Are you sure you want to change the base?
Conversation
@HannesWell please note: code in src-gen is generated and would need a change to the upstream fragment in xtext |
releng/MWE.setup
Outdated
@@ -10,7 +10,7 @@ | |||
xmlns:setup.targlets="http://www.eclipse.org/oomph/setup/targlets/1.0" | |||
xmlns:setup.workingsets="http://www.eclipse.org/oomph/setup/workingsets/1.0" |
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.
mwe.setup is not maintained for years. have no idea what the current state is
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.
Didn't look too bad, but I can check it in detail.
@HannesWell I see what you're doing here but I fail to understand the why beyond fewer deps are better Can you help me understand the reasoning? |
Yes. Can you point me to where this has to be done?
Fewer deps are better :) especially since Guava is often a very difficult dependency (one reason is xtext re-exporting it), see for example: And since the Java standard-library provides almost drop-in replacements or at least simple alternatives, I assumed this would be straight forward. |
If (and that's a big if) we (as a community) want to put effort into this, it should start with Xtext and a plan to avoid the re-export without breaking clients. That should be discussed in Xtext. |
2d9a0a7
to
8eccab4
Compare
I created eclipse/xtext#2976 for that, if I'm not mistaken, changes the template so that the LanguageActivator should not use the Guava method anymore after the next rerun of the workflow. The adjustment to not use Guava in the template for
At least I would be willing to put some effort into this. :) In order to be able to replace the remaining usages of Guava I have created the following PRs in Xtext:
I have already added a second commit that leverages those future changes which is why the build fails. If they are available I'll just squash the commits. |
Created eclipse/xtext#2990 which should cover the last part of generated code. |
8eccab4
to
3a77d49
Compare
The only remaining usage of Guava is in 'production' code is in
Mwe2ScopeProvider
becauseScopes
does not providejava.util.Function
overloads, but that probably could be added.In tests only Mwe2ContainerTest is left, which also requires a corresponding overload in Xtext.
If you are fine with that I can provide corresponding PRs.
I also adjusted a few line in generated code. Maybe Xtext can also be adapted to not reference Guava in generated code anymore?
About the change in
EcoreGenerator
I wonder if that would be considered a breaking change for MWE (because it is public)?