-
Notifications
You must be signed in to change notification settings - Fork 201
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 org.freemarker with org.freemarker.freemarker #547
Conversation
d5068af
to
54b576f
Compare
Apologies @jonahgraham I hadn't noticed the approval notification before I pushed an amendment. The amendment just adds a note to the API Changelog |
Bundle-Name: %pluginName | ||
Bundle-Vendor: %providerName | ||
Require-Bundle: org.eclipse.core.runtime, | ||
org.eclipse.core.resources, | ||
org.freemarker;visibility:=reexport, | ||
org.freemarker.freemarker;visibility:=reexport, |
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.
@jonahgraham Seems we can either add a filter to suppress the API error here (as both bundles contain the same packages so the API isn't actually broken) or we remove the re-export and bump the major version and make this a breaking API change.
Maybe a point for discussion on the call, do we want to make a breaking API change in the next release?
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.
Since we have a call today, lets discuss then - I've added to the agenda.
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.
Decision in the meeting was to update freemarker to be package imports and remove the require bundle + reexport. That may lead to a some rare breakages for anyone that was relying on the reexport, but otherwise updating freemarker bundle is identified as API breakage anyway. This needs a N&N/API entry or similar.
87a2ace
to
18ae9bb
Compare
Current draft updates all the freemarker related dependencies. Once the main branch is ready for 11.4 changes I will also remove the re-export from the tools.templates.ui plugin currently tricky to do as every bundle shows API errors due to platform related changes. I'll also add the N&N entry |
The API errors are tracked in #551 |
bda8ef3
to
8241c73
Compare
Also remove re-exports & add package imports Fixes eclipse-cdt#546
8241c73
to
646309b
Compare
@jonahgraham I think this is everything discussed in the call, but could do with another set of eyes to check I didn't miss anything. |
@Kummallinen I pushed a commit that updates all the items that depend on tools.templates to 2.0.0, I bumped the ui bundle to 2.0.0 for consistency and added an additional note to the API. Can you look and see if that looks ok to you? |
@jonahgraham Thanks, looks ok to me |
Also remove re-exports & add package imports Fixes eclipse-cdt#546
Fixes #546
This will have an impact on downstream consumers as they will now need to add their own package imports.
The target platform including the CDT feature was masking this issue earlier