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

adds missing formats in the registry #3167

Merged
merged 53 commits into from
Mar 6, 2023

Conversation

baywet
Copy link
Contributor

@baywet baywet commented Feb 20, 2023

fixes #845
closes #1811

@baywet baywet mentioned this pull request Feb 20, 2023
registries/_format/decimal.md Outdated Show resolved Hide resolved
registries/_format/base64url.md Outdated Show resolved Hide resolved
registries/_format/binary.md Show resolved Hide resolved
registries/_format/byte.md Outdated Show resolved Hide resolved
@baywet
Copy link
Contributor Author

baywet commented Feb 20, 2023

@handrews thanks for the help here. Another question/remark I had is why do we need the issue link in the table view? could we do away with it to replace the column by notes (like using the content encoding in 3.1 instead of base64url format)?

@handrews
Copy link
Member

@baywet I don't know anything about that table so someone else will have to answer the thing about the issue link. It might be good to have a notes column, or an OAS version translation column of some sort.

@baywet
Copy link
Contributor Author

baywet commented Feb 21, 2023

@baywet I don't know anything about that table so someone else will have to answer the thing about the issue link. It might be good to have a notes column, or an OAS version translation column of some sort.

Looks like this was originally added by @webron and @MikeRalphson in #1762. Any opposition to removing the Issue column to make room for a notes, parent type and version columns?

@MikeRalphson
Copy link
Member

@baywet I don't know anything about that table so someone else will have to answer the thing about the issue link. It might be good to have a notes column, or an OAS version translation column of some sort.

Looks like this was originally added by @webron and @MikeRalphson in #1762. Any opposition to removing the Issue column to make room for a notes, parent type and version columns?

I have no objection to removing the issue column from the index table, as long as the optional issue link is maintained in the detail pages.

@baywet
Copy link
Contributor Author

baywet commented Feb 21, 2023

@MikeRalphson thanks! I just pushed some changes, let me know what you think.

@MikeRalphson
Copy link
Member

@MikeRalphson thanks! I just pushed some changes, let me know what you think.

@baywet, I went to https://baywet.github.io/OpenAPI-Specification/ hoping you'd set up your branch as a GitHub pages source so we could view the built changes, and the top-level gh-pages index page is there, but https://baywet.github.io/registry/index.html 404s?

@baywet
Copy link
Contributor Author

baywet commented Feb 21, 2023

@baywet, I went to https://baywet.github.io/OpenAPI-Specification/ hoping you'd set up your branch as a GitHub pages source so we could view the built changes, and the top-level gh-pages index page is there, but https://baywet.github.io/registry/index.html 404s?

@MikeRalphson the fork had automatically setup the website. But the root is already used by my blog and it seems the template doesn't handle being on a sub segment very well. Also the branch was setup to gh_pages but I'm making my changes on a feature branch.

Bottom line, it's now refreshed at https://baywet.github.io/OpenAPI-Specification/registry/format/ and if you click on any link, the OpenApi-Specification segment will be missing, so you need to add it back.

@ralfhandl
Copy link
Contributor

@baywet Just noticed that int16 is missing here as well as in https://spec.openapis.org/registry/format/: can we close this gap between int8 and int32 with this PR?

@baywet
Copy link
Contributor Author

baywet commented Feb 22, 2023

@ralfhandl this is something I noticed as well and meant to ask when opening the PR but forgot about it.

@baywet
Copy link
Contributor Author

baywet commented Mar 6, 2023

Nice, love this! Thank you for these additions.

One thing I'd like to see as well is an iso8601 format, indicating essentially "date or date-time". Or would format: [date, date-time] work similarly to how type: [string, number] works?

@rattrayalex I don't think this is something you can do (format doesn't support arrays AFAIK), you'd have to use 2 schema entries in an oneOf IMHO

@baywet
Copy link
Contributor Author

baywet commented Mar 6, 2023

@handrews :

  • html was already here before this PR and the only changes are template changes. Considering that, do you still want me to move it to a separate PR and what's the rationale?
  • decimal. moving to a separate PR soon. (next commit)

@darrelmiller would it make sense to put "OAS" in the "source" column for the formats defined in at least one version of OAS? (presumably without linking because we don't want to get version-specific?)

Resurfacing this comment for @darrelmiller

@baywet baywet mentioned this pull request Mar 6, 2023
@darrelmiller
Copy link
Member

@darrelmiller would it make sense to put "OAS" in the "source" column for the formats defined in at least one version of OAS? (presumably without linking because we don't want to get version-specific?)

Resurfacing this comment for @darrelmiller

I guess we could. I don't feel strongly about this.

@baywet
Copy link
Contributor Author

baywet commented Mar 6, 2023

Thanks for the precision. Added.

@darrelmiller @handrews @MikeRalphson @ralfhandl We should be good for hopefully final review this time.

@MikeRalphson
Copy link
Member

For those following along, a reminder that the rendered version of this PR is here

@baywet I notice there's a blank entry in the formats table - I think it may be due to a YAML indent issue in idn-email.md?

@baywet
Copy link
Contributor Author

baywet commented Mar 6, 2023

@MikeRalphson thanks for catching that. Your suggestion appears to have fixed it.

Copy link
Member

@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.

@baywet you are correct about html, it is fine as it is! (This is why I wanted to split the changed vs new files - I know the diff shows what kind of changes there are but with a PR touching this many files that I'm half-reviewing in diff and half-reviewing in rendered, it's confusing).

I see that some RFC references are links and some are not- they should either all be links or all not be links. Aside from that, I think everything here is ready to go. I'll approve this and you and @darrelmiller can sort out what to do (or not do) about the other RFC links.

@handrews
Copy link
Member

handrews commented Mar 6, 2023

Wait, why is there still a deprecated_note with a version if @darrelmiller wanted to not list a version, and all the UI does is change it to a Yes/No in the display?

@baywet
Copy link
Contributor Author

baywet commented Mar 6, 2023

@handrews The version is not displayed anywhere at this time, it's used as a flag for the table and as a note for us in case we need to review why was this particular format marked as deprecated. The only place where people can see it is if they view the source MD. If we want to have it gone from the YAML header too (no mention of the version), I'd be happy to update it.

@darrelmiller
Copy link
Member

@handrews @baywet Sorry, I should have been more clear. I wanted to call it deprecation note to indicate that a version was not a required value to indicate that something was deprecated. e.g. If a registry entry was created because we discovered that people had using the term "flag" to indicate boolean but there was no corresponding specification, we should be able to indicate that we want it to be deprecated.
I don't have strong opinion on whether we should or shouldn't show the OAS version in the deprecation note.

I thought I had caught all the RFCs with missing links. I'll do one more sweep and if I miss any today, I'll submit a separate PR for them.

@darrelmiller darrelmiller merged commit 7592eb8 into OAI:gh-pages Mar 6, 2023
@baywet baywet deleted the feature/additional-formats branch March 6, 2023 23:58
@MikeRalphson
Copy link
Member

Thank you @baywet @handrews @ralfhandl for your herculean efforts!

@baywet
Copy link
Contributor Author

baywet commented Mar 7, 2023

Ditto! Thanks everyone for the help getting this merged!!! 🚀🚀🚀🚀

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.

7 participants