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

Allow boolean array [true, false] to be a valid argument for BooleanValidator #848

Merged
merged 6 commits into from
Apr 13, 2023

Conversation

PanosCodes
Copy link
Contributor

@PanosCodes PanosCodes commented Mar 20, 2023

How

  • Allow BooleanValidator to be build using [true, false] as argument.
  • Remove the [true, false] handling from Swagger generator

It would probably be easier to review per commit

@PanosCodes PanosCodes marked this pull request as draft March 20, 2023 18:54
@PanosCodes PanosCodes changed the title Add tests when validation type is [true, false] Allow boolean array to be a valid argument for BooleanValidator Mar 20, 2023
@PanosCodes PanosCodes force-pushed the tests-for-boolean-array branch 2 times, most recently from e528ec1 to f5317ac Compare March 20, 2023 21:10
@PanosCodes PanosCodes marked this pull request as ready for review March 20, 2023 21:13
@PanosCodes PanosCodes changed the title Allow boolean array to be a valid argument for BooleanValidator Allow boolean array [true, false] to be a valid argument for BooleanValidator Mar 21, 2023
Copy link

@mjobin-mdsol mjobin-mdsol 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, I'm slightly afraid this could introduce a breaking change... ?

@PanosCodes
Copy link
Contributor Author

PanosCodes commented Mar 22, 2023

We can always put it behing and setting for example

Apipie.configure do |config|
  config.validator.boolean.handle_boolean_array = false
end

My question is why use [true, false] in the first place instead of :bool or :boolean ?
and if we decide to allow [true, false] to be passed as validator should we also allow [1, 0]

I can't think any compelling reason to do so and I'm leaning on closing this PR 😅 (we can use the tests refactoring in another PR)

@mathieujobin
Copy link
Collaborator

I don't know, we can keep it open for a while and wait if others wants to chime in their opinion.

Copy link
Collaborator

@mathieujobin mathieujobin left a comment

Choose a reason for hiding this comment

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

if you want to solve the conflict after the refactor, I will merge this as well.

@mathieujobin
Copy link
Collaborator

Thanks, I wasn't exactly sure about the spec removal...

@mathieujobin mathieujobin merged commit b2ac28e into Apipie:master Apr 13, 2023
@PanosCodes PanosCodes deleted the tests-for-boolean-array branch April 14, 2023 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants