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

Migrate MWE (almost entirely) off Guava #287

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HannesWell
Copy link
Member

The only remaining usage of Guava is in 'production' code is in Mwe2ScopeProvider because Scopes does not provide java.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)?

@cdietrich
Copy link
Member

@HannesWell please note: code in src-gen is generated and would need a change to the upstream fragment in xtext

releng/MWE.setup Outdated Show resolved Hide resolved
@szarnekow
Copy link
Contributor

@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?

@HannesWell
Copy link
Member Author

@HannesWell please note: code in src-gen is generated and would need a change to the upstream fragment in xtext

Yes. Can you point me to where this has to be done?

@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?

Fewer deps are better :) especially since Guava is often a very difficult dependency (one reason is xtext re-exporting it), see for example:
eclipse-xtext/xtext#2671

And since the Java standard-library provides almost drop-in replacements or at least simple alternatives, I assumed this would be straight forward.

@szarnekow
Copy link
Contributor

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.

@HannesWell HannesWell marked this pull request as draft April 5, 2024 21:24
@HannesWell
Copy link
Member Author

HannesWell commented Apr 5, 2024

@HannesWell please note: code in src-gen is generated and would need a change to the upstream fragment in xtext

I created eclipse-xtext/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 Mwe2Parser is still pending.

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.

At least I would be willing to put some effort into this. :)
But I created eclipse-xtext/xtext#2975 in order to discuss that for Xtext.

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.

@HannesWell
Copy link
Member Author

@HannesWell please note: code in src-gen is generated and would need a change to the upstream fragment in xtext

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.

Created eclipse-xtext/xtext#2990 which should cover the last part of generated code.

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