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

Improve JSON-LD context export #2483

Merged
merged 4 commits into from
Dec 12, 2023
Merged

Conversation

cthoyt
Copy link
Collaborator

@cthoyt cthoyt commented Nov 29, 2023

Closes #2410 by using the @id/@prefix combination, which is required since JSON-LD technically doesn't support URI prefixes that end with an underscore _

cc @cmungall

Closes OBOFoundry#2410 by using the `@id`/`@prefix` combination, which is required since JSON-LD technically doesn't support URI prefixes that end with an underscore `_`
@matentzn
Copy link
Contributor

@jamesaoverton do you know any specific reason not to do this? It was requested by @cmungall and it is the "correct" thing to do. My gut feeling is wave it through, roll back if something breaks, but if you already kNOw something will break we might as well fix it first.

Copy link
Contributor

@balhoff balhoff left a comment

Choose a reason for hiding this comment

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

Thanks!

@jamesaoverton
Copy link
Member

This change is necessary, I guess, but quite annoying. We have a bunch of tools that predate the changes to JSON-LD that assume prefixes have to end with selected characters. We pushed back on that design decision but they ignored us.

ROBOT packages obo_context.jsonld and uses it for the default set of prefixes. I simply copied this file into ROBOT and that broke the unit test suite: ontodev/robot#1170. I haven't looked any deeper, and I won't have time for at least a week. If someone else can step up, that would be great.

Since we have to make this change, and we'll have to fix our workflows anyway, I think that this PR should also add the obo prefix as requested in #2462.

@jamesaoverton
Copy link
Member

Sorry, I was wrong. Another 5 minutes of looking into this and I see that ROBOT does not just use the obo_context.jsonld but adds several common prefixes to it. When I restored those, the test suite passed again: ontodev/robot@4f1af7e.

@jamesaoverton
Copy link
Member

Ok the upshot is that ROBOT will need a minor update to handle this change before the next release. Not a problem.

This reassures me that our workflows should not break as long as they are using JSON-LD libraries to work with the context file, and not doing it with custom code.

Copy link
Member

@jamesaoverton jamesaoverton left a comment

Choose a reason for hiding this comment

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

I tested with ROBOT and this looks good. Thanks!

Since we're already changing the context, this would be a good time to add the obo prefix, as requested in #2462.

@cthoyt
Copy link
Collaborator Author

cthoyt commented Nov 30, 2023

@jamesaoverton I am happy to add that in either this PR or a follow-up one. Maybe there are other reservations, though, based on the discussion in #2462

Copy link
Contributor

@matentzn matentzn left a comment

Choose a reason for hiding this comment

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

Lets see what breaks!

@matentzn matentzn merged commit e520f55 into OBOFoundry:master Dec 12, 2023
3 checks passed
@cthoyt cthoyt deleted the update-obo-context branch December 12, 2023 12:16
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.

JSON-LD context does not expand as intended
4 participants