-
Notifications
You must be signed in to change notification settings - Fork 45
fix: removed NameValidations to properly show default values #178
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
Conversation
it's not boolean types, but boolean-ish validations, like, say exclusiveMinimum: trueI know it's not the best example since it's draft 4, but hopefully you get the idea. |
|
|
||
| export const Validations: React.FunctionComponent<IValidations> = ({ validations, hideExamples }) => { | ||
| const numberValidations = pick(validations, numberValidationNames); | ||
| const booleanValidations = omit( |
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.
you prob want to revert it and simply add default to excluded validations
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.
that excludes it both from booleanValidations and keyValueValidations but I want them to still show in keyVal ones. But I can fix that. I'm just wondering, Is there any usecase where it makes more sense to use NameValidations rather than KeyValueValidations?
Because, if the value is true or false I think it makes more sense to display it using KeyValueValidations, so it says default: true rather than Default. Same for example or even for exclusiveMinimum (which wont be shown here because it falls in numberValidations)
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.
Is there any usecase where it makes more sense to use NameValidations rather than KeyValueValidations?
That's a good question... I'm not really sure whether we actually have any booleanish validations left now that we filter out default, readOnly, writeOnly, and everything related to numbers is handled separately.
I think it makes more sense to display it using KeyValueValidations, so it says default: true rather than Default
I agree
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.
We used NameValidations before because some of they sounded better this way (the property name was enough) it ie. deprecated, readOnly, writeOnly etc. Default can be any type, but agree that as boolean it might look weird. If it makes more sense to change it given those validations are represented differently now then go ahead, I think it's fine.
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.
Gotcha. Yea I think most of them are handled differently now (all those mentioned included)
|
Ok @P0lip @mallachari then my suggestion is to leave the PR as is, which means, in case we missed any "booleanish validation" it will just be represented with And in case there is some we are missing, we can bring it back, but I would do it explicitly this time (not by default like now) Approve if you agree π |
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.
ajmo
|
π This PR is included in version 4.3.6 π The release is available on: Your semantic-release bot π¦π |
For stoplightio/platform-internal#9043
I couldn't find the reason why
NameValidationswere used for boolean types so I removed it π€·ββοΈNow, in case of
It shows:
Rather than displaying
Defaultin casedefault: trueand nothing in case ofdefault: false.Besides that, I noticed it also fixed the issue with some other boolean types like:
Which is was previously displayed as:
@mallachari Do you remember why was
NameValidationsneeded for booleans? Is there some cases that i'm not seeing which this PR could broke?