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

(closes #555) Fall back to default logic in useNativeTypes mode for RDF numbers which are not JSON numbers #625

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

Conversation

anatoly-scherbakov
Copy link
Contributor

@anatoly-scherbakov anatoly-scherbakov commented Jan 12, 2025

This is my second attempt to fix this issue after #619.


Preview | Diff

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.

Minor changes, but this direction seems like the way to go.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@anatoly-scherbakov anatoly-scherbakov force-pushed the 555-blind-spot-in-rdf-to-object-conversion branch from fd7684b to 3b61a1b Compare January 13, 2025 19:52
index.html Outdated Show resolved Hide resolved
@BigBlueHat BigBlueHat added the class-3 Class-3 change label Jan 15, 2025
@anatoly-scherbakov
Copy link
Contributor Author

anatoly-scherbakov commented Jan 15, 2025

  • Todo: implement <ins> & <del> tags

@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#625

anatoly-scherbakov: this is the new version of w3c/json-ld-api#619 ; thanks gkellogg and pchampin for your suggestions.

gkellogg: it's a fairly small change.
… However, it is not purely editorial, so it should be turned into a candidate amendment, with <ins>s, <del>s...
… It has to be done manually, not much automation is available for that.

anatoly-scherbakov: I can not wrap a whole block in <ins> or <del>, right?

bigbluehat: correct, they are 'inline'. Is there a specific class to use?

gkellogg: there are other things to put in place. Respec documentation for them is not great, but there are examples in the same doc.

anatoly-scherbakov: what is the use-case for this marking?

gkellogg: it is there for reviewers of the specification, because we are editing a published Recommendation.

anatoly-scherbakov: an HTML diff will not be enough for the reviewers?

gkellogg: no, they do not expect to look at HTML raw code.
… You can look at the W3C process documents that describes the requirements for these things.

<gkellogg> https://www.w3.org/policies/process/#candidate-amendments

bigbluehat: this is required until we recharter. The alternative is to keep a bunch of open PR and merge them only after we recharter.
… I don't know why TallTed's tick is not green.
… In the DID WG, we are using a list of code owners.


@gkellogg
Copy link
Member

There’s additional markup needed to show the controls for candidate change ins/del.

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 the other markup necessary, and called it Candidate Correction 5.

@anatoly-scherbakov
Copy link
Contributor Author

Thanks for reviews!

I still can't merge the PR though 🤔

image

@gkellogg gkellogg force-pushed the 555-blind-spot-in-rdf-to-object-conversion branch from 62e7730 to 33a652b Compare January 27, 2025 22:44
@gkellogg
Copy link
Member

Needs a review from @pchampin.

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.

Sorry for the delay. I'm approving this PR because the spec text now looks good, and to avoid more delay in back-and-forth...

However, the new test must be amended per my comment, as currently it is both buggy and non-compliant.

Comment on lines +35 to +38
},
{
"@type": "http://www.w3.org/2001/XMLSchema#decimal",
"@value": 1.1
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
},
{
"@type": "http://www.w3.org/2001/XMLSchema#decimal",
"@value": 1.1

This must be removed for two reasons:

  • the spec only tells to convert xsd:integer and xsd:double, and for a good reason (see below), so this part of the test is actually not complying with the spec;

  • xsd:decimals should never be converted to JSON native number because they do not round-trip: this one is converted back to "1.1E0"^^xsd:decimal, which is ill-formed (xsd:decimal does not support scientific notation in its lexical space).

<http://example.com/boolean-object> <http://example.com/example> "False"^^<http://www.w3.org/2001/XMLSchema#boolean> .

<http://example.com/number-native> <http://example.com/example> "1"^^<http://www.w3.org/2001/XMLSchema#integer> .
<http://example.com/number-native> <http://example.com/example> "1.1"^^<http://www.w3.org/2001/XMLSchema#decimal> .
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
<http://example.com/number-native> <http://example.com/example> "1.1"^^<http://www.w3.org/2001/XMLSchema#decimal> .

Must be removed (see my comment on 0027-out.jsonld below).

Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

Tiny language fixes

@@ -5456,6 +5456,12 @@ <h3>Algorithm</h3>
<li>Initialize <var>converted value</var> to <var>value</var>.</li>
<li>Initialize <var>type</var> to <code>null</code></li>
<li>If <a data-link-for="JsonLdOptions">useNativeTypes</a> is <code>true</code>
<div class="candidate correction" id="change_5">
<span class="marker">Candidate Correction 5</span>
<p>This changes the behavior of using native numbers when <a data-link-for="JsonLdOptions">useNativeTypes</a> is `true`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p>This changes the behavior of using native numbers when <a data-link-for="JsonLdOptions">useNativeTypes</a> is `true`.
<p>This changes behavior when using native numbers where <a data-link-for="JsonLdOptions">useNativeTypes</a> is `true`.

<ol>
<li>
Attempt to convert the <a>lexical form</a> to a <a>JSON number</a>
according to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
according to
according to the

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
class-3 Class-3 change
Projects
Development

Successfully merging this pull request may close these issues.

blind spot in RDF to object conversion
6 participants