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

Fix issue 622 #623

Merged
merged 3 commits into from
Jan 27, 2025
Merged

Fix issue 622 #623

merged 3 commits into from
Jan 27, 2025

Conversation

multimeric
Copy link
Contributor

@multimeric multimeric commented Dec 14, 2024

Closes #622.


Preview | Diff

@BigBlueHat BigBlueHat added the class-3 Class-3 change label Jan 15, 2025
@gkellogg gkellogg added class-2 Class-2 change and removed class-3 Class-3 change labels Jan 15, 2025
@w3cbot
Copy link

w3cbot commented Jan 15, 2025

gkellogg marked as non substantive for IPR from ash-nazg.

index.html Outdated Show resolved Hide resolved
@gkellogg gkellogg added class-3 Class-3 change and removed class-2 Class-2 change labels Jan 15, 2025
@w3cbot
Copy link

w3cbot commented Jan 15, 2025

pchampin marked as substantive for IPR from ash-nazg.

@w3cbot
Copy link

w3cbot commented Jan 15, 2025

This was discussed during the #json-ld meeting on 15 January 2025.

View the transcript

w3c/json-ld-api#623

<gb> Pull Request 623 Fix issue 622 (by multimeric)

bigbluehat: it looks like a white space reformat

gkellogg: it should have a change class label.

TallTed: it's more than just white space.

<niklasl> It says to fix w3c/json-ld-api#622 which is labelled class-3

<gb> Issue 622 Issue with Step 6 of `9.4.1 LoadDocumentCallback` (by multimeric) [spec:bug] [ErratumRaised] [class-3]

bigbluehat: good point niklasl.

gkellogg: this is class-2, it is not substantive.

TallTed: I disagree. The previous one said "X hold. reject", now it says "IF X holds, then reject".

gkellogg: ok, yes, it is class-3

pchampin: I'm a bit concerned we say this is both non-substantive + normative
… we don't not (yet) have an IPR commitment from this person
… but it seems we need it

gkellogg: we need to mark it back as substantive

pchampin: I'll see what I can do.
… I found it, marked it back as substantive, and sent a message to the person for them to commit to the IPR policy.

bigbluehat: ok, let's wait for their response.
… This change seems good, nobody seems to objet to it.

gkellogg: we need some tests for this.

<gkellogg> w3c/json-ld-api#622

<gb> Issue 622 Issue with Step 6 of `9.4.1 LoadDocumentCallback` (by multimeric) [spec:bug] [ErratumRaised] [class-3]

bigbluehat: anyone willing to write tests?

[crickets]

bigbluehat: ok, I'm marking it as "needs-test".


@pchampin
Copy link
Contributor

thanks @multimeric for the IPR commitment.
@BigBlueHat @gkellogg we are good to move forward with this PR.

@gkellogg gkellogg requested a review from pchampin January 25, 2025 23:30
Copy link
Member

@gkellogg gkellogg left a comment

Choose a reason for hiding this comment

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

I added Candidate Correction markup and a log entry.

Copy link
Contributor

@pchampin pchampin left a comment

Choose a reason for hiding this comment

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

There is a spurious change in the "changelog" section, which should be removed from this PR.
Otherwise, LGTM.

index.html Outdated
Comment on lines 6915 to 6917
<li>2025-01-25: Correct some corner cases in transforming RDF Number Literals
to JSON numbers when <a data-link-for="JsonLdOptions">useNativeTypes</a> is `true`,
as described in <a href="#change_5">Candidate Correction 5</a>.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<li>2025-01-25: Correct some corner cases in transforming RDF Number Literals
to JSON numbers when <a data-link-for="JsonLdOptions">useNativeTypes</a> is `true`,
as described in <a href="#change_5">Candidate Correction 5</a>.</li>

I suspect this has leaked from another PR, right?
It should probably be removed from that one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added in 3b0a95c which I didn't make. Do you want me to fix it?

Copy link
Member

Choose a reason for hiding this comment

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

I added it to simplify a future merge, otherwise, it will lead to a conflict. If we do the PRs in an appropriate order, these will mesh together.

@gkellogg gkellogg merged commit f004001 into w3c:main Jan 27, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with Step 6 of 9.4.1 LoadDocumentCallback
6 participants