dashbitco / nimble_options

A tiny library for validating and documenting high-level options. πŸ’½
Apache License 2.0
507 stars 38 forks source link

Add a `:type_spec` option #120

Closed whatyouhide closed 10 months ago

whatyouhide commented 10 months ago

I'd love to have this for a bunch of use cases (mostly {:custom, ...} types). Thoughts?

github-actions[bot] commented 10 months ago

Pull Request Test Coverage Report for Build 44c00153f8d848fcfc5fb171e9d9b8fbc713b8a6-PR-120


Totals Coverage Status
Change from base Build 46e70d7e890bb40faa7be672430c052c19046c0f: 0.02%
Covered Lines: 277
Relevant Lines: 298

πŸ’› - Coveralls
josevalim commented 10 months ago

I think once we start adding quoted expressions, we are probably already doing too much. So I am :-1. It is better to specify the type separate and then you tell us which type to use.

whatyouhide commented 10 months ago

@josevalim the repetition if you have to do this yourself is quite a bit. For example, I have a schema with ~20 options, and I only need to customize the type spec of a couple of those. Given the amount of code this adds to nimble_options, and given this is (I think?) the only thing left that you can't do with this library, I think this makes sense as an addition.

josevalim commented 10 months ago

The issue is that it comes with downsides. For example, you can't add docs to the types themselves. And when clicking to go to source, it will point to a meta-programmed source. Sharing types require going back to usual types instead of using this option. Etc. So I can easily see this cascading into a bunch of options and controls to customize code generation, or lead into an inconsistent style where some types are written in NimbleOptions, and others aren't. This will be more inconsistent than consistent.

whatyouhide commented 10 months ago

you can't add docs to the types themselves. And when clicking to go to source, it will point to a meta-programmed source.

I’m not sure what you mean here @josevalim, sorry 😬 Since we have NimbleOptions.option_typespec/1, which does generate code for the spec of an option ({:opt1, type1} | ...), I figured this would be a low-hanging-fruit type of addition.

josevalim commented 10 months ago

Gah, I forgot about NimbleOptions.option_typespec/1. Should we at least replace type_doc then?

whatyouhide commented 10 months ago

@josevalim you mean that type_spec should be exclusive with type_doc? If so yeah I could get behind that πŸ˜‰

josevalim commented 10 months ago

I don’t see the benefit of keeping type_doc around. It is an inferior version of type_spec which does not generate the correct spec.

whatyouhide commented 10 months ago

@josevalim ah, yeah! You're totally right. I shall update this PR to deprecate type_doc (we can't remove it, we're past 1.0) and replace it with type_spec.

whatyouhide commented 10 months ago

@josevalim no, wait. Edit. Take this case:

nodes: [
  type_doc: "list of `t:String.t/0`",
  type_spec: quote(do: [String.t()])
]

If we want to auto-generate a type doc from type_spec where I can click stuff around, this is gonna be messy. Plus, with type_doc I can easily describe types (through words) that are harder to express with specs ("non-empty map oft:atom/0to etc."). So, I’m thinking maybe the two can coexist after all...

josevalim commented 10 months ago

Gah. Alright, let's have this in, but to me this is the last step on the slippery-slope of typespecs in NimbleOptions. :D If we need further customization, i would rather yank the whole typespec generation out.

whatyouhide commented 10 months ago

@josevalim deal πŸ˜‰ Slippery slope indeed, I’m kind of wishing we didn't have option_typespec/1 too now. Regrets, man. At least this PR is like 25 LOC :)