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

Add support for PartialRangeUpTo in validators #2369

Merged
merged 4 commits into from Jun 1, 2020

Conversation

PopFlamingo
Copy link
Contributor

Up until right now, the .count(...) and .range(...) validators didn't support being passed a PartialRangeUpTo. This commit adds support for this.

This also improves and fixes documentation

Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!!

A couple nits ('cause it wouldn't be me if I didn't 🙂):

  • Unless I missed them (very possible!), there don't seem to be any tests for PartialRangeFrom, can you add a couple?

  • Could you add a more-generic overload for accepting any RangeExpression not already covered by the specializations?

    • For the count() validators, something like public static func count<R: RangeExpression>(_ range: R) where R.Bound == Int { .count(range.relative(to: .zero ... .max)) } (give or take a generic constraint maybe).

    • For the range() validators, it'd go in a new extension that requires T: Comparable & FixedWidthInteger conformance, be where R.Bound == T, and look something like .range(range.relative(to: .min ... .max)), I think.

These are completely optional, though; the PR is already great as-is! ❤️

@gwynne gwynne added enhancement New feature or request semver-minor Contains new API labels May 26, 2020
@gwynne gwynne added this to Awaiting Review in Vapor 4 via automation May 26, 2020
@PopFlamingo
Copy link
Contributor Author

@gwynne Thanks for the review! 🙂

Could you add a more-generic overload for accepting any RangeExpression not already covered by the specializations?

I didn't know about this protocol, unfortunately it seems like you can't use a range with .relative(to:): only Collections are accepted (and it uses the collection indices to limit the range! I'll see what a custom collection could do).

@PopFlamingo
Copy link
Contributor Author

Unless I missed them (very possible!), there don't seem to be any tests for PartialRangeFrom, can you add a couple?

Done! 🙂

@gwynne gwynne requested a review from tanner0101 May 26, 2020 20:29
@gwynne
Copy link
Member

gwynne commented May 26, 2020

I didn't know about this protocol, unfortunately it seems like you can't use a range with .relative(to:): only Collections are accepted (and it uses the collection indices to limit the range! I'll see what a custom collection could do).

Nah, don't worry about it - the primary utility of RangeExpression is in generically accepting all of the existing range types plus any custom kinds that might be in use. Having thought about it further, I think these APIs are better off staying specific about which types of range they will accept. For example, you theoretically could set it up so you could specify an UnboundedRange (the ... expression) for a count or range validator, but that's an awful lot of work to make the compiler and runtime do just to validate that yes, the value being checked is indeed a value! 😆

I'm happy with this as it now stands. Ping @tanner0101 to give it a glance before it gets merged.

@PopFlamingo
Copy link
Contributor Author

@gwynne I saw your comment too late 😃 I can still remove it if that's better without it

@PopFlamingo PopFlamingo changed the title Add support for PartialRangeUpTo in .count(...) and .range(...) validators Add support for PartialRangeUpTo and RangeExpression in .count(...) and .range(...) validators May 26, 2020
@PopFlamingo PopFlamingo changed the title Add support for PartialRangeUpTo and RangeExpression in .count(...) and .range(...) validators Add support for PartialRangeUpTo and RangeExpression in validators May 26, 2020
@gwynne
Copy link
Member

gwynne commented May 29, 2020

@adtrevor I like what you did to implement that! But yeah, I think we're better off without it conceptually as well as for simplicity. Once you revert the commits to add the RangeExpression support, I'll merge this. Apologies for sending you down the rabbit hole only to change my mind - I hadn't thought it through at first 😰

@PopFlamingo PopFlamingo changed the title Add support for PartialRangeUpTo and RangeExpression in validators Add support for PartialRangeUpTo in validators Jun 1, 2020
@PopFlamingo
Copy link
Contributor Author

@gwynne No problem! 🙂 Reverted!

@gwynne gwynne merged commit 4fa7ed4 into vapor:master Jun 1, 2020
Vapor 4 automation moved this from Awaiting Review to Done Jun 1, 2020
@tanner0101
Copy link
Member

These changes are now available in 4.8.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semver-minor Contains new API
Projects
Vapor 4
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants