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 a :type_spec option #120

Merged
merged 2 commits into from
Dec 12, 2023
Merged

Add a :type_spec option #120

merged 2 commits into from
Dec 12, 2023

Conversation

whatyouhide
Copy link
Collaborator

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

Copy link

github-actions bot commented Dec 10, 2023

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

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 92.953%

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

💛 - Coveralls

@josevalim
Copy link
Member

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
Copy link
Collaborator Author

@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
Copy link
Member

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
Copy link
Collaborator Author

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
Copy link
Member

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

@whatyouhide
Copy link
Collaborator Author

@josevalim you mean that type_spec should be exclusive with type_doc? If so yeah I could get behind that 😉

@josevalim
Copy link
Member

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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

@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 of t:atom/0 to etc."). So, I’m thinking maybe the two can coexist after all...

@josevalim
Copy link
Member

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
Copy link
Collaborator Author

@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 :)

@whatyouhide whatyouhide merged commit f17c624 into main Dec 12, 2023
2 checks passed
@whatyouhide whatyouhide deleted the al/typespecs branch December 12, 2023 08:09
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.

2 participants