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

Adding convenience range validators. #4

Merged
merged 2 commits into from Jan 13, 2018
Merged

Adding convenience range validators. #4

merged 2 commits into from Jan 13, 2018

Conversation

LucianoPAlmeida
Copy link
Contributor

@LucianoPAlmeida LucianoPAlmeida commented Jan 13, 2018

Just a few convenience options to validate ranges :)

@codecov-io
Copy link

codecov-io commented Jan 13, 2018

Codecov Report

Merging #4 into master will increase coverage by 11.96%.
The diff coverage is 97.91%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master       #4       +/-   ##
===========================================
+ Coverage   85.24%   97.21%   +11.96%     
===========================================
  Files          32       32               
  Lines        1159     1077       -82     
  Branches      158        0      -158     
===========================================
+ Hits          988     1047       +59     
+ Misses         31       30        -1     
+ Partials      140        0      -140
Impacted Files Coverage Δ
Quiver/Validating/Extensions/Validator+Range.swift 100% <100%> (ø) ⬆️
...alidating Tests/QuiverIntegerValidatorsTests.swift 100% <100%> (+26.02%) ⬆️
...Validating/Default Validators/RangeValidator.swift 81.25% <80%> (-5.42%) ⬇️
Quiver/Validating/ValidatorEngine.swift 84.31% <0%> (-6.01%) ⬇️
...Validating/Default Validators/RegexValidator.swift 84.61% <0%> (-4.28%) ⬇️
...ating/Default Validators/ComparatorValidator.swift 84.61% <0%> (-3.96%) ⬇️
...alidating/Default Validators/LengthValidator.swift 86.66% <0%> (-3.34%) ⬇️
Quiver/Validating/Validatable.swift 100% <0%> (ø) ⬆️
...idating/Default Validators/RequiredValidator.swift 100% <0%> (ø) ⬆️
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b669d9...3d18b73. Read the comment docs.

@heitorgcosta
Copy link
Owner

I don't think there's a need for a validator for all ranges when all of the range types conforms to the RangeExpression protocol. You can write only one validator with T: RangeExpression and the value to be used in the range~=value expression can be casted into T.Bound, which is a comparable.

Something like this:
class RangeTypeValidator<T>: Validator where T: RangeExpression
the init should be modified to:
init(range: T) instead of init(range: Range<T>)
and in the validation process:
guard let value = object! as? T.Bound else {}
and the object propertie should be a RangeExpression instead of Range<T>

On the convenience methods in the Validator's extensions, only one method is needed:
`public static func range<T: RangeExpression>(_ range: T, message: String? = nil) -> Validator

With these changes, all tests will probably be still passable, with less code and still working.

If you could try that, I would be appreciate :3

Copy link
Owner

@heitorgcosta heitorgcosta left a comment

Choose a reason for hiding this comment

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

Nice work ✨

@heitorgcosta heitorgcosta merged commit db5c319 into heitorgcosta:master Jan 13, 2018
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.

None yet

3 participants