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 validation type to NimbleOptions.ValidationError #134

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

anthonator
Copy link
Contributor

@whatyouhide initial attempt at adding failed validation to NimbleOptions.ValidationError. Only implemented support for {:type, :integer} so I can get a sanity check on whether you think I'm on the right path.

@coveralls
Copy link

Pull Request Test Coverage Report for Build d7df5c28720d1edd68b0d059fcf4569b14733347-PR-134

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-1.7%) to 91.753%

Files with Coverage Reduction New Missed Lines %
lib/nimble_options.ex 5 91.89%
Totals Coverage Status
Change from base Build d42dc90bcfa8a2b6c1f082fb47869e4769f9e9de: -1.7%
Covered Lines: 267
Relevant Lines: 291

💛 - Coveralls

@anthonator anthonator changed the title Support validation type for :integer Add validation type to NimbleOptions.ValidationError Aug 24, 2024
@whatyouhide
Copy link
Collaborator

Yeah I think this is the right path but if we pass {:type, _} to structured_error_tuple then we don't need to pass say "integer" as well, we can move the building of the error message completely to that function and deduce the error from {:type, _} in that case.

@anthonator
Copy link
Contributor Author

if we pass {:type, _} to structured_error_tuple then we don't need to pass say "integer" as well

This is a good idea. I'll do this. Thanks!

I'll continue going down this route. Thank you!

@anthonator
Copy link
Contributor Author

Sorry for not getting back to this sooner!

@whatyouhide a couple things.

  1. After digging into this more I'm not sure how specifying {:type, _} allows us to not pass the human-readable version of the type to structured_error_tuple. It makes sense for a type like :integer but not :pos_integer. We could pattern match on the type to specify the human-readable version but I feel like that adds a ton of code without much benefit. I'm assuming I'm not thinking about this correctly or I'm missing the bigger picture.

  2. How should we handle types like {:struct, struct_name}? I think we should pass {:struct, struct_name} as the type rather than just :struct. So the validation would be {:type, {:struct, struct_name}}. Thoughts?

@whatyouhide
Copy link
Collaborator

Instead of building the error message where you validate the type, you can build it (by pattern matching) in structured_error_tuple. That was my idea by it's been a while so I’m not really sure here anymore 😄

@anthonator
Copy link
Contributor Author

Thanks for the clarification! That's what I assumed you were referring to. Here is what I wrote in my original question.

We could pattern match on the type to specify the human-readable version but I feel like that adds a ton of code without much benefit.

Would you agree with that?

@whatyouhide
Copy link
Collaborator

Yeah this is why I wanted a structured error reason that we could turn into a message later on. Ok, for now let's keep this version passing strings around and I'll see if I can refactor that at some point!

@anthonator
Copy link
Contributor Author

How should we handle types like {:struct, struct_name}? I think we should pass {:struct, struct_name} as the type rather than just :struct. So the validation would be {:type, {:struct, struct_name}}. Thoughts?

@whatyouhide
Copy link
Collaborator

Mh I can't recall if :struct is a type or if it's {:struct, struct_mod}. Either way, yeah a separate PR to clean that up if it's weird would be welcome 🙃

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