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

Adds case(of:) Validation #2172

Merged
merged 17 commits into from
Feb 19, 2020
Merged

Adds case(of:) Validation #2172

merged 17 commits into from
Feb 19, 2020

Conversation

mohpor
Copy link
Contributor

@mohpor mohpor commented Feb 10, 2020

Added Enum validation for RawRepresentable enums. Checks to see if a value can be represented as a specific enum, and if enum is CaseIterable, reports the possible values too.

e.g.:

final class TestModel: Content, Validatable {
  enum CustomEnum: String, Content, CaseIterable {
    case test, ok
  }

  let type: CustomEnum

  static func validations(_ validations: inout Validations) {
    validations.add("type", as: String.self, is: .case(of: CustomEnum.self))
  }
}

Signed-off-by: Mohammad <porooshani@gmail.com>
Signed-off-by: Mohammad <porooshani@gmail.com>
@mohpor
Copy link
Contributor Author

mohpor commented Feb 12, 2020

@tanner0101 I can't find any circleci config files. What's happening?!

@Joannis
Copy link
Member

Joannis commented Feb 12, 2020

@mohpor CircleCI is triggering on Vapor 4 because Vapor 3 still uses CircleCI. It's an issue between the two testing setups.

@tanner0101 tanner0101 added the enhancement New feature or request label Feb 13, 2020
@tanner0101 tanner0101 added this to In Progress in Vapor 4 via automation Feb 13, 2020
Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

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

+1 to this change, thank you.

Sources/Vapor/Validation/Validators/Case.swift Outdated Show resolved Hide resolved
Sources/Vapor/Validation/Validators/Case.swift Outdated Show resolved Hide resolved
Tests/VaporTests/ValidationTests.swift Outdated Show resolved Hide resolved
}

public var failureDescription: String? {
var message = "value {\(rawValue)} cannot be represented as \(E.self)."
Copy link
Member

Choose a reason for hiding this comment

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

The name of the enum seems like an implementation detail to me. This failure description should be similar to the .in validator and say something like "value is not foo, bar, or baz"

Copy link
Contributor Author

@mohpor mohpor Feb 15, 2020

Choose a reason for hiding this comment

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

You're right. It's better that way. Applicable only to CaseIterables. But still.
We are going to need a better error message for non-CaseIterables.

Tests/VaporTests/ValidationTests.swift Outdated Show resolved Hide resolved
@mohpor mohpor changed the title Added CaseOf Validation Added case(of: ) Validation Feb 15, 2020
@tanner0101 tanner0101 changed the title Added case(of: ) Validation Added case(of:) Validation Feb 15, 2020
@tanner0101
Copy link
Member

@mohpor if you rebase your branch off latest master the circle ci build error will be fixed 👍

@mohpor
Copy link
Contributor Author

mohpor commented Feb 16, 2020

@mohpor if you rebase your branch off latest master the circle ci build error will be fixed 👍

Done.

mohpor and others added 11 commits February 18, 2020 09:04
Signed-off-by: Mohammad <porooshani@gmail.com>
Signed-off-by: Mohammad <porooshani@gmail.com>
Signed-off-by: Mohammad <porooshani@gmail.com>
* Adds a generic EndpointCache<T> class

* Review fixups

* Changed internal to private

* Changed internal to private

* Renamed the error

* add EndpointCache tests

* add CacheControl convienience init

* rename badJSON error case

Co-authored-by: Tanner <me@tanner.xyz>
Signed-off-by: Mohammad <porooshani@gmail.com>
Signed-off-by: Mohammad <porooshani@gmail.com>
Signed-off-by: Mohammad <porooshani@gmail.com>
@mohpor
Copy link
Contributor Author

mohpor commented Feb 18, 2020

Sorry about the mess. Should be squashed.

@tanner0101
Copy link
Member

No problem, GitHub will squash them for us.

Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

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

Thanks for the latest changes! This looks great. Just minor nits left.

Sources/Vapor/Validation/Validators/Case.swift Outdated Show resolved Hide resolved
if cases.count > 1 {
suffix = " or \(cases.removeLast())"
}
return "is not \(cases.joined(separator: ", "))\(suffix)."
Copy link
Member

Choose a reason for hiding this comment

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

Most annoying nit of all time: can we have an oxford comma here? >_<

You should be able to copy makeDescription from the .in validator (In.swift) which supports both successDescription and failureDescription and includes the comma:

    func makeDescription(not: Bool) -> String {
        let description: String
        switch self.items.count {
        case 1:
            description = self.items[0].description
        case 2:
            description = "\(self.items[0].description) or \(self.items[1].description)"
        default:
            let first = self.items[0..<(self.items.count - 1)]
                .map { $0.description }.joined(separator: ", ")
            let last = self.items[self.items.count - 1].description
            description = "\(first), or \(last)"
        }
        return "is\(not ? " not" : " ") \(description)"
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I was thinking how inefficient is in's description generation! >_<

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a matter of fact, the in's description message has an invalid 2-space distance for successful cases.

@tanner0101 tanner0101 added the semver-minor Contains new API label Feb 18, 2020
Signed-off-by: Mohammad <porooshani@gmail.com>
Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

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

Thanks!

@tanner0101 tanner0101 changed the title Added case(of:) Validation Adds case(of:) Validation Feb 19, 2020
@tanner0101 tanner0101 merged commit 71cd6a7 into vapor:master Feb 19, 2020
Vapor 4 automation moved this from In Progress to Done Feb 19, 2020
@tanner0101
Copy link
Member

These changes are now available in 4.0.0-beta.3.22

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

4 participants