Skip to content

New self-identification proposal #3989

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

Merged
merged 3 commits into from
Aug 7, 2024
Merged

New self-identification proposal #3989

merged 3 commits into from
Aug 7, 2024

Conversation

handrews
Copy link
Member

@handrews handrews commented Aug 1, 2024

Without the bundling this time.
For OAS 3.2 and Arazzo 1.1.

Without the bundling this time.
For OAS 3.2 and Arazzo 1.1.
@handrews handrews added this to the v3.2.0 milestone Aug 1, 2024
@handrews handrews requested review from a team as code owners August 1, 2024 16:05
Copy link
Contributor

@jeremyfiel jeremyfiel left a comment

Choose a reason for hiding this comment

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

couple nits.

Looks good and the proposal is clear. Nice job!


As [OAS 3.1.1 clarifies](https://github.com/OAI/OpenAPI-Specification/pull/3758), it is already mandatory to separate location and identity for Schema Object support.

Currently, associating a URI other than the current URL with a document to meet this requirement has to be done externally. Many tools effectively by allowing the retrieval URL to be set manually, without verifying that the document actually lives at the given URL.
Copy link
Contributor

Choose a reason for hiding this comment

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

Many tools effectively by allowing the retrieval...

I think you missed something here


### Plain name fragments in every Object

While including `self` in every Object would produce the same complexity as JSON Schema's nested `$id`, we could just adopt an equivalent of JSON Schema's `$anchor` keyword, which (like HTML/XML's `id` attribute) creates a plain name fragment that is not tied to the location of the Object in the JSON/YAML structure.
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
While including `self` in every Object would produce the same complexity as JSON Schema's nested `$id`, we could just adopt an equivalent of JSON Schema's `$anchor` keyword, which (like HTML/XML's `id` attribute) creates a plain name fragment that is not tied to the location of the Object in the JSON/YAML structure.
Including `self` in every Object would produce the same complexity as JSON Schema's nested `$id` but, that introduces unnecessary schema bloat rather than a single identifier at the root.
Adopting an equivalent of JSON Schema's `$anchor` keyword, which like (HTML/XML's `id` attribute), creates a plain name fragment that is not tied to the location of the Object in the JSON/YAML structure.


While including `self` in every Object would produce the same complexity as JSON Schema's nested `$id`, we could just adopt an equivalent of JSON Schema's `$anchor` keyword, which (like HTML/XML's `id` attribute) creates a plain name fragment that is not tied to the location of the Object in the JSON/YAML structure.

Handling this would require scanning all Objects for the keyword prior to declaring that a reference target with a plain name fragment cannot be resolved. This would likely be done on document load, but could be deferred and done incrementally as-needed when unknown fragments are encountered.
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
Handling this would require scanning all Objects for the keyword prior to declaring that a reference target with a plain name fragment cannot be resolved. This would likely be done on document load, but could be deferred and done incrementally as-needed when unknown fragments are encountered.
Handling `$id` or `$anchor` requires scanning all Objects for a defined keyword prior to declaring that a reference target with a plain name fragment cannot be resolved. This would likely be done on document load, but could be deferred and done incrementally as-needed when unknown fragments are encountered.

Copy link
Contributor

@ralfhandl ralfhandl left a comment

Choose a reason for hiding this comment

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

+1, only minor nits

Incorporates some ideas from @jeremyfield (thanks!)
Copy link
Member Author

@handrews handrews left a comment

Choose a reason for hiding this comment

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

@jeremyfiel I've updated everything that I think was needed for clarity. I am aware that you dislike my writing style, but as this is not even specification text, unless and until someone writes a style guide for proposals, I'm going to stick with writing in my own voice. In some cases I reworded thing to incorporate what I felt were the bits that needed clarifying while sticking closer to my preferred wording.

@jeremyfiel
Copy link
Contributor

I am aware that you dislike my writing style

That's definitely not my intention and I understand this is not specification text. I only provided nits on clarifications I thought would enhance the understanding. (at the very least, my own)
Forgive my impression that I dislike your writing style. I don't have any qualms against it :)

ralfhandl
ralfhandl previously approved these changes Aug 2, 2024
@handrews
Copy link
Member Author

handrews commented Aug 2, 2024

@jeremyfiel

Forgive my impression that I dislike your writing style. I don't have any qualms against it :)

Of course- in that case please forgive my over-reading of whatever past comment made me think such a thing :)

I only provided nits on clarifications I thought would enhance the understanding. (at the very least, my own)

In some cases in this PR I couldn't really tell any difference, and it felt like you just wanted a different tone or preferred different words, which is the other reason I got the impression of just wanting a different style of writing. I didn't really want to go critique each suggestion, so I was hoping that I got enough of what really wasn't clear.

Sometimes if the suggested change is not an obvious problem (e.g. duplicated word, or a sentence that clearly makes no sense like the one you pointed out had something missing) it might be better to start a conversation about what you find confusing. Then there's a little more flexibility in how to solve it.

@ralfhandl ralfhandl requested review from jeremyfiel and a team and removed request for jeremyfiel August 5, 2024 08:18
mikekistler
mikekistler previously approved these changes Aug 5, 2024
Copy link
Contributor

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@ralfhandl ralfhandl dismissed stale reviews from mikekistler and themself via 6238600 August 5, 2024 13:35
@ralfhandl ralfhandl requested review from mikekistler, a team and jeremyfiel August 5, 2024 13:35
@handrews
Copy link
Member Author

handrews commented Aug 7, 2024

Since @mikekistler 's approval was only cleared because @ralfhandl merged Mike's suggestion and then approved it himself, I'm considering this to have two TSC approvals. Since all other commenters have since approved, I'm going to merge this.

@handrews handrews merged commit 873d41d into OAI:main Aug 7, 2024
2 checks passed
@handrews handrews deleted the self-id-320 branch August 13, 2024 19:26
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.

4 participants