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

An example that correctly extracts the specs from '@spec'! #19

Closed
wants to merge 1 commit into from

Conversation

Qqwy
Copy link
Owner

@Qqwy Qqwy commented Jul 11, 2020

It also does not break the documentation :-D.

Related to #17 .
Before this can be merged, we definitely need the following:

  1. An option added to use TypeCheck that enables this behaviour. Maybe read_attributes: true (defaults to false)?
  2. An option added to use TypeCheck that explicitly not writes types/specs to the type-attributes for when someone wants to customize them separately. Maybe write_attributes: false (defaults to true)?
  3. Remove the example-file before merging.

It also does not break the documentation :-D.
@baldwindavid
Copy link
Contributor

This is awesome! I've already tried it in my codebase for a number of files and it seems to work.

If we can use @spec/@type and it supports all of the standard typespec syntax (eventually) plus the special powers your library already provides, the spec/type syntax no longer needs to be used, right? This would be even better than I might have envisioned since there is no new keyword at all.

Additionally, I'm going to play devil's advocate here even though I likely don't understand all of your goals or some of the ramifications, but are 1 and 2 really necessary? Isn't it enough flexibility that you can choose to use TypeCheck on a specific module as needed already? I just wonder if additional config options might make for more confusion.

@Qqwy
Copy link
Owner Author

Qqwy commented Jul 11, 2020

I can envision cases where the attributes the user would like to have different values in the documentation vs. for the typechecking, if only because it is difficult to have a 100% feature-parity between Elixir's builtin typespecs and TypeCheck (although I want to try to reach this). For instance, I don't yet have a good idea of how to tackle #5.
EDIT: Yet another reason is that overriding @ will pose conflicts if there is another library that wants/needs to override it as well, so we need to provide a manual fallback.

However, if the code of this branch works properly (i.e. if there are no edge-cases that we've missed so far), I think that read_attributes: true will probably become be the default when someone calls use TypeCheck.

@baldwindavid
Copy link
Contributor

I continue to love working with @spec and @type from this branch. One thing I was thinking about was whether this might create any confusion where someone either:

  1. Doesn't know their use of @spec is subject to TypeCheck
  2. Thinks @spec is subject to TypeCheck, but it is not

I've already had a few times where I thought I was using TypeCheck, but then quickly realized my tests were passing with incorrect types and that I had just forgotten to use TypeCheck. Not a big deal because I suspect similar modules in a lot of codebases will use a shared module that brings TypeCheck in.

All that being said, part of me wonders if it is more explicit if, say, @spec!/@type! were used specifically when you want TypeCheck to do it's thing and then @spec/@type stay as they are. This might eliminate some of the need for some of these configuration switches as well and make it easier for existing codebases using typespecs to gradually switch in an explicit way (I know the configuration toggles also help with this though). And then if the library ever gets to the point where it can fully handle an existing codebase with @spec/@type, that option is added. And then it eventually goes into Elixir core (muhahahah).

Anyway, just some rambling thoughts.

@Qqwy
Copy link
Owner Author

Qqwy commented Jul 15, 2020

All that being said, part of me wonders if it is more explicit if, say, @spec!/@type! were used specifically when you want TypeCheck to do it's thing and then @spec/@type stay as they are.

This is a great idea! I think it is a perfect marriage between explicitness (not altering existing behaviour) and the 'principle of least surprise' (people will know how it probably works without having to look it up).

I'll alter this PR to work with @spec! and @type! instead, and will rename the macros to reflect this change as well.

@Qqwy
Copy link
Owner Author

Qqwy commented Jul 18, 2020

Closed in favour of #23

@Qqwy Qqwy closed this Jul 18, 2020
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.

None yet

2 participants