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

Begin prototyping data types of fields and optional #173

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

thadguidry
Copy link
Contributor

@thadguidry thadguidry commented Apr 14, 2024

Fixes #157

  • I'm not 100% sure if we should link using ecmascript, or another standard for the data types. WhatWG has Infra https://infra.spec.whatwg.org/#string as an example, but then I didn't see Array. Still it would be nice to link and point to a standard, but that would be JSON, and I couldn't figure out how to make the ReSpec shortcut links work with RFC8259 and so resorted to the shortcut links using ecmascript (but this all could be changed I'm sure if we knew more or had a friend in ReSpec land)
  • Anyways, take a look at the Core Concepts section Entities where I begin to add them along with some extra info Terms Used and references we were missing about JSON standards in general that we talked about throughout the spec.

- I'm not 100% sure if we should link using ecmascript, or another standard for the data types.  WhatWG has Infra https://infra.spec.whatwg.org/#string as an example, but then I didn't see Array.  Still it would be nice to link and point to a standard, but that would be JSON, and I couldn't figure out how to make the ReSpec shortcut links work with RFC8259 and so resorted to the shortcut links using ecmascript (but this all could be changed I'm sure if we knew more or had a friend in ReSpec land)
Copy link

netlify bot commented Apr 14, 2024

Deploy Preview for reconciliation-api-specs ready!

Name Link
🔨 Latest commit a42b465
🔍 Latest deploy log https://app.netlify.com/sites/reconciliation-api-specs/deploys/66e39cdce4a8a10007ffca31
😎 Deploy Preview https://deploy-preview-173--reconciliation-api-specs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@thadguidry thadguidry requested a review from fsteeg April 14, 2024 08:05
@thadguidry thadguidry marked this pull request as ready for review April 14, 2024 08:05
Copy link
Member

@fsteeg fsteeg left a comment

Choose a reason for hiding this comment

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

Formatting looks good, gives nice overview of data type & optional vs. required.

Made one inline suggestion. Also ReSpec complains about the unused required definition. I think omitting required looks good though, so maybe we can just remove the dfn and make it bold instead? Or link to it once from some place?

draft/index.html Outdated Show resolved Hide resolved
thadguidry and others added 2 commits April 16, 2024 10:08
Co-authored-by: Fabian Steeg <[email protected]>
- Also remove duplicate phrasing paragraph for JSON convention used
@thadguidry
Copy link
Contributor Author

Formatting looks good, gives nice overview of data type & optional vs. required.

Thanks!

Made one inline suggestion. Also ReSpec complains about the unused required definition. I think omitting required looks good though, so maybe we can just remove the dfn and make it bold instead? Or link to it once from some place?

Yeah, agree. So, instead, I now have just put a comment inside doc about that, and then made it only bold/italic to match.
Also wrapped paratheses around them to showcase the syntax used in the document. I think this makes things easier to distinguish.

Finally, does anyone have opinions on the linking for the data types? Notice, if you hover or click on "String" or "Array" etc. that it's currently linked to "ecmascript". Is that OK you think? or should we link to some other standard available in xref ?

@thadguidry
Copy link
Contributor Author

I'll instead link the Data Types against the RFC8259 JSON Specification

<dt><code>description</code></dt>
<dd>{{String}} ({{optional}}) a <emph>description</emph> as a human-readable string;</dd>
<dt><code>type</code></dt>
<dd>{{Array}} ({{optional}}) list of <a>types</a>, possibly empty, the entity is classified by;</dd>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fsteeg Does this appended phrase make things more clear, or does it assume too much? I felt like it was missing this context, which might not be apparent to developers outside of Semantic Web or Knowledge Graph circles (or just getting introduced to them) when we're talking about Entities and their Types.

@thadguidry
Copy link
Contributor Author

@fsteeg

  1. So I layered in the JSON spec conventions (and removed the ecmascript xref since we agreed to conform to just JSON spec), and then with those defined, I now began to add more datatypes. How do things look now (links look/feel good) (format is just fine)?
  2. Also took the liberty to slightly clarify a few things and improve grammar. One thing I am wondering about is clarifying the Entities - type, where I appended ... the entity is classified by. Does that make good sense?

@fsteeg
Copy link
Member

fsteeg commented Sep 16, 2024

Looks good to me in principle. Seeing the type directly makes it clearer. It feels a bit like there's a delimiter (e.g. a dot?) missing between the type and the description, but maybe it's fine. I think the description should be consistently capitalized though.

@thadguidry
Copy link
Contributor Author

@fsteeg Let's just take this as an example to come to consensus, and to ensure I understand what you are asking for:

<dd>{{Array}} ({{optional}}) list of <a>types</a>, possibly empty, the entity is classified by;</dd>

  1. You want a dot between the datatype (or optional) and the description? Such as:
    <dd>{{Array}} ({{optional}}). List of <a>types</a>, possibly empty, the entity is classified by;</dd>
  2. So you think descriptions should always start with a captial such as "list" should be "List" ?

@fsteeg
Copy link
Member

fsteeg commented Sep 17, 2024

Yes, that's what I meant.

Copy link
Member

@wetneb wetneb left a comment

Choose a reason for hiding this comment

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

I'm not super fond of redefining the JSON concepts. Intuitively we should be able to just link to whatever spec defines that and avoid making our spec even longer (I think it's already a bit scary because of its length).

Concerning the formatting of field types, it feels to me that we are missing a sort of delimiter between the field type and its description. An alternative way of formatting this would be to have the field name, type, optional status and description all gathered in a table.

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.

Use some better HTML/Markdown format structure to show which query response fields are optional
3 participants