vex
vex copied to clipboard
CargoSense
Add type validator.
Hi,
I would like to validate the types of the values in a struct, but I could not find any easy
way to do it, so I added a validator to check for types.
It works with all builtin types and can be used to check complex types %{:atom => binary}, or things like this.
Take a look at the docs examples and test cases for more info.
Thank you!
Hey there!
First I want to say that this is an impressive endeavor to embark upon. You're basically retro-fitting static type analysis into the language, and that can be a real challenge. You've done a very good job so far.
I'll tell you that I am at the moment a little skeptical that we can make this work in a sufficiently generic way. We know a thing or two about type systems in elixir from our work on doing GraphQL (http://absinthe-graphql.org/), and it can be a rather hairy process.
For now, the first roadblock is nil. At the moment nil will simply get treated as an atom, but that isn't quite right. You probably need to allow nil to be a value in a type list, that indicates that the type can be nil. IE %{:any => [:binary, nil]} would allow nil values, whereas %{:any => :binary} would not.
From there the next thing people are going to want is the ability to specify particular values. You can do it in elixir typespec with atoms e.g @type foo :: :foo | :bar | :baz and people will no doubt want it here if we're doing type support. This gets a bit challenging because at the moment we're representing types via atoms, but those same atoms might also get used as values.
Then we're in trouble because the moment we allow values into the system we gotta worry about mandatory vs optional values for our container types, and so on.
You see why I'm a bit concerned about trying to support this.
What are your thoughts @bruce ?
Hi @benwilson512,
Thank you very much for the detailed feedback! I know that type checking is not an easy task, but in the case of this library, I think it could be very useful. I also think it should be enough for a good percentage of most people use cases, the most common one IMO clearly being to check if a top level value is or not of the right type.
For now, the first roadblock is nil. At the moment nil will simply get treated as an atom, but that isn't quite right. You probably need to allow nil to be a value in a type list, that indicates that the type can be nil. IE %{:any => [:binary, nil]} would allow nil values, whereas %{:any => :binary} would not.
You are perfectly right. I was using allow_nil for my particular case, but this will clearly
not work for nested types. Making nil a special case should be easy enough.
From there the next thing people are going to want is the ability to specify particular values. You can do it in elixir typespec with atoms e.g @type foo :: :foo | :bar | :baz and people will no doubt want it here if we're doing type support. This gets a bit challenging because at the moment we're representing types via atoms, but those same atoms might also get used as values.
The current approach will clearly not fit this case. For top level values, the inclusion validator
will be good enough, but this will not work for nested values either.
I however think this limitation is acceptable, and if someone needs this level of control on nested values,
the best bet should still be to use another validator.
Then we're in trouble because the moment we allow values into the system we gotta worry about mandatory vs optional values for our container types, and so on.
Wouldn't a proper nil support be enough to handle this? I would be glad if you could give me an example.
I'll update the PR with proper nil handling for now, and let's continue the discussion.
I just updated the PR with proper nil handling.
I also removed the possibility of using the allow_nil option, as the nil type should be used instead in this validator.
Let me know what you think.
Then we're in trouble because the moment we allow values into the system we gotta worry about mandatory vs optional values for our container types, and so on.
Wouldn't a proper
nilsupport be enough to handle this? I would be glad if you could give me an example.
Not if we allow values. Allowing values would let us say something like "not, only must you supply a map with atom keys, the only valid keys :foo, :bar". This is a lot like @type config :: [foo: any, bar: any]. Of course at this point we end up in a discussion of how we say "keys :foo, :bar are REQUIRED to be present, and :baz is optional. This isn't quite as simple as allowing :bar to point to a nil value because pointing to nil isn't the same as the key being absent.
I do agree that trying to deal with values is probably beyond us here.
@benwilson512 Thanks for the feedback!
I get your point about allowing values, but I really think it would make things too complex if we try to handle this in this validator. For map or keyword, which is I think the most common place to use values, a specialized validator would be much simpler from an implementation point of view, and easy enough to use IMO. What do you think?
By the way, to give a real use case, I am currently planning to use the type validator to avoid sending invalid JSON data. https://github.com/tuvistavie/pushex/blob/master/lib/pushex/gcm/request.ex
As a total side note, if you used GraphQL not only would your JSON output be automatically type checked, but your JSON input from clients would be too ;)
@benwilson512 Thank you for the review! For my above use case a validator will be good enough, but I have some JSON API I am working on, so I will give GraphQL there :+1: I will update the PR with your feedback and post again!
Is there still interest in merging this? I could review and merge this.
I have been using this as a custom validator in one of my libraries [1] for quite a while now, so if you could review and merge it, it would be great! Thanks [1]: https://github.com/tuvistavie/pushex
Won't have much time in the next few weeks, sorry. I'll update when I can, but it might take a little while.