Skip to content

Conversation

@domagojk
Copy link
Contributor

@domagojk domagojk commented Nov 29, 2021

For stoplightio/platform-internal#9043

I couldn't find the reason why NameValidations were used for boolean types so I removed it πŸ€·β€β™‚οΈ

Now, in case of

"defaultExample1": {
  "type": "boolean",
  "default": true
},
"defaultExample2": {
  "type": "boolean",
  "default": false
}

It shows:

Screenshot 2021-11-29 at 15 02 44

Rather than displaying Default in case default: true and nothing in case of default: false.

Besides that, I noticed it also fixed the issue with some other boolean types like:

"something": {
  "type": "boolean",
  "example": true
}

Screenshot 2021-11-29 at 15 04 25

Which is was previously displayed as:

Screenshot 2021-11-29 at 15 06 02

@mallachari Do you remember why was NameValidations needed for booleans? Is there some cases that i'm not seeing which this PR could broke?

@domagojk domagojk requested a review from mallachari November 29, 2021 14:10
@domagojk domagojk marked this pull request as ready for review November 29, 2021 14:10
@domagojk domagojk requested review from a team and mnaumanali94 November 29, 2021 14:10
@P0lip
Copy link
Contributor

P0lip commented Nov 29, 2021

boolean types

it's not boolean types, but boolean-ish validations, like, say

exclusiveMinimum: true

I 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(
Copy link
Contributor

@P0lip P0lip Nov 29, 2021

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

Copy link
Contributor Author

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)

Copy link
Contributor

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

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.

Copy link
Contributor Author

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)

@domagojk
Copy link
Contributor Author

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 KeyValueValidations rather than NameValidations.

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 πŸ˜„

@domagojk domagojk requested a review from P0lip November 30, 2021 11:17
Copy link
Contributor

@P0lip P0lip left a comment

Choose a reason for hiding this comment

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

ajmo

@domagojk domagojk merged commit 686400b into master Nov 30, 2021
@domagojk domagojk deleted the fix/boolean-default-value branch November 30, 2021 18:32
@stoplight-bot
Copy link
Collaborator

πŸŽ‰ This PR is included in version 4.3.6 πŸŽ‰

The release is available on:

Your semantic-release bot πŸ“¦πŸš€

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants