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

add tests for rdf:JSON xsd:float and xsd:double #152

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

Conversation

pfps
Copy link

@pfps pfps commented Nov 20, 2024

Tests for rounding of numbers and treatment of large numbers.

Tests for difference between positive and negative zero.

Tests for order dependence of rdf:JSON arrays and order independence of rdf:JSON objects.

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.

They look basically correct, but there are some syntax errors and places where you may be using the wrong datatype.

rdf/rdf12/rdf-semantics/json-array-1.ttl Outdated Show resolved Hide resolved
rdf/rdf12/rdf-semantics/json-array-1.ttl Outdated Show resolved Hide resolved
rdf/rdf12/rdf-semantics/json-array-2.ttl Outdated Show resolved Hide resolved
rdf/rdf12/rdf-semantics/json-positive-zero-array.ttl Outdated Show resolved Hide resolved
mf:action <json-array-1.ttl>;
mf:entailmentRegime "RDF";
mf:name "json-array-unordered";
mf:recognizedDatatypes (rdf:JSON);
Copy link
Contributor

Choose a reason for hiding this comment

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

mf:recognizedDatatypes needs to go into /ns/test-manifest.ttl.

That in turn changes the meaning of passing a test.

trs:json-array-ordered a mf:NegativeEntailmentTest;
rdfs:comment "Arrays are ordered in rdf:JSON.";
mf:action <json-array-1.ttl>;
mf:entailmentRegime "RDF";
Copy link
Contributor

Choose a reason for hiding this comment

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

mf:entailmentRegime needs to in /ns/test-manifest.ttl.

mf:name "json-object-unordered";
mf:recognizedDatatypes (rdf:JSON);
mf:result <json-object-2.ttl>;
mf:unrecognizedDatatypes ();
Copy link
Contributor

Choose a reason for hiding this comment

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

mf:unrecognizedDatatypes needs to go into /ns/test-manifest.ttl.

Copy link
Author

Choose a reason for hiding this comment

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

These IRIs have been in the semantics tests since at least 1.1. I wonder how the 1.1 testing was done.

Copy link
Member

Choose a reason for hiding this comment

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

Test runners don't often actually require the vocabulary terms to be defined, but it is good form to define all the terms used in tests, and pretty much everywhere else.

mf:unrecognizedDatatypes ();
test:approval test:NotClassified .


Copy link
Contributor

Choose a reason for hiding this comment

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

Excess blank line.

Copy link
Author

Choose a reason for hiding this comment

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

How do I remove a line here?

Copy link
Contributor

Choose a reason for hiding this comment

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

To keep it as one commit to be merged:

  • If changes have been made via teh github UI, "git pull" to you local copy of the branch.
  • Edit file.
  • git commit

Bonus points for doing a "squash and merge" to turn it into one commit going onto to main.

Copy link
Member

Choose a reason for hiding this comment

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

You can also add multiple suggestions into a batch in the UI which will create just one commit. This is done from the "Files changed" view and you'll see a button to "add to batch".

Copy link
Contributor

Choose a reason for hiding this comment

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

/ns/test-manifest.ttl needs updating. I've picked out some terms that need to be defined - there may be others.

@pfps
Copy link
Author

pfps commented Nov 21, 2024

Is all OK now?

#153 has been opened for the IRIs missing in test-manifest.ttl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants