-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
Format Registry documentation is not accurate / well published #3789
Merged
handrews
merged 4 commits into
OAI:gh-pages
from
Bellangelo:fix-base-type-array-joining
May 11, 2024
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could create some confusion. The intent in this particular case is that the
sf-integer
format can be applied to eithertype: integer
ortype: number
. Since OAS 3.1 thetype
value can be an array, so a reader might think thattype: [integer, number]
is the base type for this format.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on what we use more it looks like this is indeed the correct format. You can check also decimal and decimal128.
One more note, I am not so sure that the representations in these files are THAT important. The reason is that these files seem to just be for building the page and not for documentation. Which means that we mostly care for the end result and how the data will appear when rendered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this:
and thanks for the link to the
decimal
format because that shows how problematic the change actually is.I would argue that this is not how we want the data to appear, so the array format just looks wrong to me.
If anything, I think we should be going the other direction, to make all these just a comma-separated string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikekistler That's what the PR fixes. They will appear as
string, number
. I don't fully agree that the users might be confused but if others agree with you we can change the others instead of thesf-integer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bellangelo I think what @mikekistler is getting at is that we just need the existing
string, number
to be rendered asstring, numer
, rather than rendering it as something that looks like a tuple with first a string and then a number.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @handrews and @mikekistler, I think there is a misunderstanding here, so I will try to explain the situation the best I can.
So, the line that @mikekistler commented on is a variable for the Liquid template engine that Jekyll uses. The reason why the
sf-integer
and thedecimal
appear different is that thesf-integer
sets the variable type to a string, while the other to an array. Liquid by default joins array's item together without any separator. This is solved by this line which overrides the default behavior and instead uses the separator that we want, a comma with a space.Did you test it and had different results?
While we could easily just use the string instead, so keeping the
sf-integer
as is and changing the rest, the reason I persist on this is that it feels kind of weird to me that the produced JSON would have abase_type
which is astring
but it contains actually 2 values.From this point, there are 2 ways to go to:
PS. Sorry for insisting on a minor issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bellangelo Ah, yes I see, thank you for clarifying! I agree that sf-integer should be an array here, (matching how it is in decimal, for example). I got confused thinking that all of the others did not wrap that list in an array. But they do. My apologies! I read through this too quickly.
@mikekistler I think this is correct, or at least it's the same as the other formats that use either of two types. If this isn't the way we want to structure type alternative lists, we should probably sort that out separately from this bugfix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extended explanation @Bellangelo! This looks good then.