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

GH-2199 exclude jackson dependencies from POM #2200

Merged

Conversation

barthanssens
Copy link
Contributor

GitHub issue resolved: #2199

Briefly describe the changes proposed in this PR:

  • exclude jackson dependencies from rio-api

(I added the json-ld java to support user-specified DocumenLoader a few releases ago, but forgot to exclude the unused dependencies)


PR Author Checklist:

  • my pull request is self-contained
  • I've added tests for the changes I made
  • every commit message starts with the issue number (GH-xxxx) followed by a meaningful description of the change
  • every commit has been signed off

Note: we merge all feature pull requests using squash and merge. See RDF4J git merge strategy for more details.

@barthanssens barthanssens added the ⛔ Not backwards compatible A change that breaks backwards compatibility and needs a major release label May 12, 2020
Copy link
Contributor

@abrokenjester abrokenjester left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand the change to be honest. Are you saying you can run the jsonld-java parser/writer without having these jackson libraries available? If so, what does jsonld-java use to (de)serialize json?

Edit never mind, I realize now that you are only using the jsonld-java dependency in rio-api to get access to one particular interface/class, not to actually run the tool, so you don't need its third party deps here.

Copy link
Contributor

@abrokenjester abrokenjester left a comment

Choose a reason for hiding this comment

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

LGTM. I'm on the fence whether we should really count this as incompatible - arguably it fixes a bug in our dependency structure and I don't quite count relying on buggy behavior as something we should provide compatibility for. Though I realize that if you stretch that reasoning you can call any change a bug :)

@barthanssens
Copy link
Contributor Author

It's a feature ! :-)
I don't really mind to merge it into the next 3.3 version instead of 4 (though in that case it's probably wise to add a small comment to the release notes)

@abrokenjester
Copy link
Contributor

I don't really mind to merge it into the next 3.3 version instead of 4 (though in that case it's probably wise to add a small comment to the release notes)

Excellent idea. That also ties in with something @hmottestad suggested earlier: that we start early on writing our release notes, so that we can all contribute to them. I'll put up a PR on rdf4j-docs with a new draft for the 3.3 release notes, and we can all push to that branch as we go along making further changes.

@barthanssens barthanssens force-pushed the GH-2199-rio-jackson-dependencies branch from 30be999 to dc2713c Compare May 28, 2020 08:29
Signed-off-by: Bart Hanssens <bart.hanssens@bosa.fgov.be>
@barthanssens barthanssens force-pushed the GH-2199-rio-jackson-dependencies branch from c9845b9 to dc9c65e Compare May 28, 2020 10:02
@barthanssens barthanssens merged commit 1d17aa6 into eclipse-rdf4j:develop May 28, 2020
@barthanssens barthanssens deleted the GH-2199-rio-jackson-dependencies branch October 1, 2020 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⛔ Not backwards compatible A change that breaks backwards compatibility and needs a major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants