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

Make reference to JSO informative by explaining the data qualities #37

Merged
merged 9 commits into from Oct 25, 2021

Conversation

cabo
Copy link
Member

@cabo cabo commented Oct 22, 2021

No description provided.

@cabo cabo requested review from akeranen and mjkoster Oct 22, 2021
Copy link
Collaborator

@akeranen akeranen left a comment

I think this change makes sense. A set of small change suggestions inline.

sdf.md Outdated Show resolved Hide resolved
sdf.md Outdated Show resolved Hide resolved
sdf.md Outdated
constrains the data value to be an integer multiple of the number
given.
(Type "`integer`" can also be expressed as a "`multipleOf`" quality of
value 1, unless another "`multipleOf`" quality is present.
Copy link
Collaborator

@akeranen akeranen Oct 23, 2021

Choose a reason for hiding this comment

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

This might confuse more than clarify. I would probably leave this sentence out.

And missing closing ")".

Copy link
Member Author

@cabo cabo Oct 23, 2021

Choose a reason for hiding this comment

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

I find that piece of information useful; it makes the reader think, which is often a prerequisite to understanding.

The length (as measured in characters) can be constrained by the
additional data qualities "`minLength`" and "`maxLength`", which are
inclusive bounds.
Note that the previous version of the present document explained
Copy link
Collaborator

@akeranen akeranen Oct 23, 2021

Choose a reason for hiding this comment

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

Instead of talking about the mistake in the previous version, I'd suggest to say here just something along the lines of:

Note that the length of the string in characters is not necessarily the same as in octets unless specific encoding that has this feature (e.g., UTF-8) is used.

Copy link
Member Author

@cabo cabo Oct 23, 2021

Choose a reason for hiding this comment

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

Er, UTF-8 does not have the same number of bytes as it encodes characters.

sdf.md Outdated
The data quality "`pattern`" takes a string value that is interpreted
as an [ECMA-262] regular expression in Unicode mode that constrain the
string (note that these are not anchored by default, so unless `^` and
`$` anchors are employed they match any string that *contains* a match
Copy link
Collaborator

@akeranen akeranen Oct 23, 2021

Choose a reason for hiding this comment

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

It wasn't obvious from first read where "they" refers to, so I was thinking of suggesting here "the expressions", but not sure if that's actually much better.

describe entries in the specified JSON object: The key gives an
allowable map key for the specified JSON object, and the value is a
map with a named set of data qualities giving the type for the
corresponding value in the specified JSON object.
Copy link
Collaborator

@akeranen akeranen Oct 23, 2021

Choose a reason for hiding this comment

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

Here we might need an example

Copy link
Member Author

@cabo cabo Oct 23, 2021

Choose a reason for hiding this comment

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

... which are all over the document, no?

sdf.md Outdated Show resolved Hide resolved
sdf.md Outdated
| required | array of strings | (object) names of properties (note 2) that are required in the JSON map ("object") |
| properties | named set of data qualities | (object) entries allowed for the JSON map ("object") |
{: #sdfdataqual1 title="Qualities of sdfProperty and sdfData borrowed from json-schema.org"}

(1) A type value of `integer` means that only integral values of JSON
Copy link
Collaborator

@akeranen akeranen Oct 23, 2021

Choose a reason for hiding this comment

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

Given the table is removed, I guess so should be the notes?

Copy link
Member Author

@cabo cabo Oct 23, 2021

Choose a reason for hiding this comment

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

Yes.

sdf.md Outdated
(1) A type value of `integer` means that only integral values of JSON
numbers can be used.

(2) Note that the term "properties" as used for map entries in {{-jso}} is unrelated to sdfProperty.
(2) Note that the term "properties" as used for map entries in JSO is unrelated to sdfProperty.
Copy link
Collaborator

@akeranen akeranen Oct 23, 2021

Choose a reason for hiding this comment

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

(same as above)

Copy link
Member Author

@cabo cabo Oct 23, 2021

Choose a reason for hiding this comment

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

Yep.

sdf.md Outdated
@@ -823,32 +838,10 @@ homogeneous arrays of numbers, text, Booleans, or maps. (This list might be
extended in a future version of SDF.) An "allowed value" is a value
Copy link
Collaborator

@akeranen akeranen Oct 23, 2021

Choose a reason for hiding this comment

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

I think this parts needs update based on the removed table below (e.g., "allowed types" not used anymore).

Copy link
Member Author

@cabo cabo Oct 23, 2021

Choose a reason for hiding this comment

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

The CDDL, where this term continues to be used, should be self-explanatory (as well as more precise) here, so I have removed this as well.

cabo and others added 5 commits Oct 23, 2021
Co-authored-by: Ari Keränen <ari.keranen@gmail.com>
Co-authored-by: Ari Keränen <ari.keranen@gmail.com>
Co-authored-by: Ari Keränen <ari.keranen@gmail.com>
sdf.md Outdated
@@ -83,6 +85,14 @@ informative:
# VERSION 2.1.2 | April 2020
Copy link
Contributor

@WAvdBeek WAvdBeek Oct 25, 2021

Choose a reason for hiding this comment

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

v2.1.4 August 2021

@WAvdBeek
Copy link
Contributor

@WAvdBeek WAvdBeek commented Oct 25, 2021

Hi Carsten,
just for my understanding, this is describing json inside of this doc instead of referencing it correct?
no obvious issues with that, if that helps the progress in IETF.

@cabo
Copy link
Member Author

@cabo cabo commented Oct 25, 2021

JSON is defined in RFC 8259 (STD 90), but this indeed pulls in definitions of the keywords that we have borrowed from the json-schema.org proposals. Note that OCF is based on Swagger 2.0 which appears to be based on "draft-4" (draft‑wright‑json‑schema‑00 actually, which is draft-5, which is draft-4 pretty much anyway), so exclusiveM..imum has a different meaning there, but apart from that change (which we have agreed a while ago) JSO keywords from OCF swagger should simply continue to just drop into SDF.

JSO-based keywords are also used in the specification techniques of a
number of ecosystems, but some adjustments may be required.

E.g., {{OCF}} is based on Swagger 2.0 which appears to be based on
Copy link
Contributor

@WAvdBeek WAvdBeek Oct 25, 2021

Choose a reason for hiding this comment

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

I am fine with having implementation notes.

not sure if we have to call out the exclusive/min/max thought since the conversion already handles this.

Copy link
Member Author

@cabo cabo Oct 25, 2021

Choose a reason for hiding this comment

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

It is mostly a template for the paragraphs that we may need to write for other ecosystems.
(And, I think, it is good to know that there are minor tweaks, even if they already are implemented.)

@cabo cabo merged commit 3d1402d into master Oct 25, 2021
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.

None yet

4 participants