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-1755 Revert #4580

Merged
merged 5 commits into from May 22, 2023
Merged

GH-1755 Revert #4580

merged 5 commits into from May 22, 2023

Conversation

hmottestad
Copy link
Contributor

@hmottestad hmottestad commented May 21, 2023

GitHub issue resolved: #4586

Briefly describe the changes proposed in this PR:

Revert changes from GH-1755


PR Author Checklist (see the contributor guidelines for more details):

  • my pull request is self-contained
  • I've added tests for the changes I made
  • I've applied code formatting (you can use mvn process-resources to format from the command line)
  • I've squashed my commits where necessary
  • every commit message starts with the issue number (GH-xxxx) followed by a meaningful description of the change

Copy link
Contributor

@erikgb erikgb left a comment

Choose a reason for hiding this comment

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

Added some comments. Some might be a bit opinionated.... 😄

return reference;
}
}
///*******************************************************************************
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe delete this file instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes it much easier to revert my revert when I can simply uncomment the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

}

}
///*******************************************************************************
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe delete this file instead?

@@ -83,4 +83,11 @@ public class BinaryRDFWriterSettings {
private BinaryRDFWriterSettings() {
}

static {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it better to have a test for this? I am not a big fan of Java asserts....

@@ -35,9 +35,9 @@
import org.eclipse.rdf4j.rio.RDFParser;
import org.eclipse.rdf4j.rio.helpers.AbstractRDFHandler;
import org.eclipse.rdf4j.rio.helpers.BasicParserSettings;
import org.eclipse.rdf4j.rio.helpers.NTriplesParserSettings;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional? I thought we were just reverting the JSON-LD changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. All the tests are reverted so that I could be sure that there wasn't anything I had missed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that makes sense, thanks!

import org.eclipse.rdf4j.rio.helpers.SimpleParseLocationListener;
import org.eclipse.rdf4j.rio.helpers.StatementCollector;
import org.eclipse.rdf4j.rio.ntriples.NTriplesParserSettings;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional? I thought we were just reverting the JSON-LD changes.

@@ -81,9 +81,6 @@ public Stream<ShapesGraph> getAllShapeContexts() {

List<? extends Statement> collect = stream.collect(Collectors.toList());

if (collect.size() > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this belong is this PR? I prefer self-contained PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be in it's own PR.

@hmottestad hmottestad merged commit d736b85 into main May 22, 2023
7 checks passed
@hmottestad hmottestad deleted the GH-1755-revert branch May 22, 2023 06:06
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.

The JSON-LD enum changes in GH-4332 are not backwards compatible
2 participants