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

address #630 #638

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

address #630 #638

wants to merge 5 commits into from

Conversation

pchampin
Copy link
Contributor

@pchampin pchampin commented Feb 27, 2025

I'm not entirely satisfied with the addition in common/terms.html, as it seems too detailed for the terminology section. But...

...as pointed out in the comment of #630,
the API spec references to this definition in multiple places, and many of those links are actually for instances of the internal representation, not for instances of the strings or maps that represent term definition in JSON.

So the alternative would be to introduce a new name for those "internal term definition" and patch all the algorithms... which seems laborious and error-prone.

@gkellogg I don't remember what's the process for synchronizing the files in common with other specs.


Preview | Diff

I'm not entirely satisfied with the addition in common/terms.html,
as it seems too detailed for the terminology section. But...

...as pointed out in the comment of #630,
the API spec references to this definition in multiple places,
and many of those links are actually for instances of the internal representation,
*not* for instances of the strings or maps that represent term definition in JSON.

So the alternative would be to introduce a new name for those "internal term definition"
and patch all the algorithms... which seems laborious and error-prone.

@gkellogg I don't remember what's the process for synchronizing the files in common with other specs.
@gkellogg
Copy link
Member

@gkellogg I don't remember what's the process for synchronizing the files in common with other specs

Just needs to be copied. The GH Action checks out common and compares the results.

Co-authored-by: Ted Thibodeau Jr <[email protected]>
@pchampin
Copy link
Contributor Author

pchampin commented Feb 28, 2025

This one probably also needs to be formatted as a candidate amendment...
And a corresponding one will be needed in json-ld-syntax.

I didn't follow the pattern 'change_N' for the id,
because this change will also be present in json-ld-syntax (in comment/terms),
and so will probably require a candidate addition in that spec too (with a different index).
Hence the more robust id used here.
@pchampin pchampin added the class-2 Class-2 change label Feb 28, 2025
Comment on lines +386 to +387
<ins cite="#change_api_638"><br/>
For <a data-cite="JSON-LD11-API#context-processing-algorithms">context processing</a>, <a>term definition</a> values are converted internally to a dedicated data structure that is easier to process.</ins>
Copy link
Member

Choose a reason for hiding this comment

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

The problem with this is that that will show up in syntax and framing, as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that's why I named it change_api_638 insteal of change_8. That way, we can add a change marker in the other specs without an ID clash.

Copy link
Member

Choose a reason for hiding this comment

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

We'll need issues for those other specs to have their own version of this update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Co-authored-by: Ted Thibodeau Jr <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
class-2 Class-2 change
Projects
Status: Discuss-Call
Development

Successfully merging this pull request may close these issues.

4 participants