Skip to content

Conversation

eroux
Copy link
Contributor

@eroux eroux commented Aug 22, 2017

This is the final PR for me. @hmottestad I've integrated and completed your PR (#207) in it. It doesn't do anything with the @requireAll flag, but at least it should have a less buggy behavior. The flag is passed as an argument of filterNode in JsonLdApi.java, but this function requires some heavy lifting to properly align with the Frame Matching algorithm of JSON-LD 1.1. If you want to really handle the flag, that's where you need to change things.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 88.657% when pulling 54e2a28 on BuddhistDigitalResourceCenter:fix-new into 5589124 on jsonld-java:master.

@eroux
Copy link
Contributor Author

eroux commented Aug 22, 2017

@ansell I think the Changelog could be clearer if it was organized by version instead of date, and was in a separate file. I tend to follow these guidelines which really help me... (an example), but that not crucial.

Copy link
Member

@ansell ansell left a comment

Choose a reason for hiding this comment

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

Just need to preserve the custom test added in #168, otherwise looks good.

},
"@type": "ex:Library",
"ex:contains": {
"@explicit":true,
Copy link
Member

Choose a reason for hiding this comment

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

My impression was that this test file was intended to be a custom test. However, it seems to have mistakenly landed in core/src/test/resources/json-ld.org/ instead of core/src/test/resources/custom. The PR which included it was #168 by @hmottestad for background reference.

Could you move it to core/src/test/resources/custom as part of this PR?

@eroux
Copy link
Contributor Author

eroux commented Aug 23, 2017

Thanks for your comments, the test is back as a custom test

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 88.719% when pulling 227461c on BuddhistDigitalResourceCenter:fix-new into 5589124 on jsonld-java:master.

@eroux
Copy link
Contributor Author

eroux commented Aug 23, 2017

The error in travis-ci is a network error and, I think, can be ignored (nothing to do with this PR)

@ansell
Copy link
Member

ansell commented Aug 23, 2017

Thanks, the changes look good and the Travis-CI error retrieving from purl.org is intermittent (it worked for JDK8), and not likely related to JDK9 in that case given it worked previously. I will merge this in and release 0.11.0 today.

@ansell ansell merged commit 227461c into jsonld-java:master Aug 23, 2017
@eroux
Copy link
Contributor Author

eroux commented Aug 24, 2017

Thanks, great to see a new release!

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