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

Options for enabling/disabling Gradient on specific files or umbrella apps #123

Closed
japhib opened this issue Aug 24, 2022 · 7 comments
Closed
Labels
enhancement New feature or request

Comments

@japhib
Copy link
Contributor

japhib commented Aug 24, 2022

What would you think about adding some more fine-grained options for where to run Gradient? I'm working on enabling Gradient type-checking for a very large umbrella project for work, and it'd be very helpful for adoption if it could be enabled for one app at a time, rather than for the entire project at once.

I think the easiest approach would probably be to allow some :gradient config inside each umbrella app's mix.exs file. That would make it easy to enable/disable Gradient for each umbrella app from within that app itself.

Another feature that is related and would be quite nice to have is the ability to enable or disable Gradient/Gradualizer for specific files within these apps. A magic comment like # gradient-disable or # gradient-enable would be great for this -- Credo does something similar with # credo:disable-for-next-line.

Besides being able to disable Gradient checking on files that have a bunch of unfixed type errors, this feature would also be nice while Gradient is still in alpha/beta to simply be able to turn off type checking for files that Gradient is having issues with. For example, I'm currently getting warnings from Gradient on files that mainly consist of a use statement at the top; when I disassemble the compiled versions of these files, there's a large amount of generated code, and the error comes from somewhere in there.

Ideally these options could have values that would work together to customize the amount of type-checking that occurs for a file and/or app. I see a sub-app within an umbrella app going through several phases of type-checking completeness during the adoption process:

  1. App is skipped completely
  2. App is skipped by default, but some files are starting to get fixed, and those files opt-in to being type checked
  3. App is enabled by default, and remaining un-fixed files opt-out of being type checked
  4. App is completely type checked

Personally, my preference would be for Gradient to default to checking all umbrella sub-apps, but each sub-app could have its own config to choose explicitly whether to be enabled or skipped. And maybe also a separate config for whether to allow files to opt in or out via magic comments.

What do you think of this idea? I understand it goes against the goal of "type-check all functions with specs." However, I think it'd be worthwhile to have these options available for large projects with lots of incorrect specs, such as the one I'm working on. It would definitely help the gradual adoption process.

@japhib
Copy link
Contributor Author

japhib commented Sep 8, 2022

@erszcz What do you think of this?

@japhib
Copy link
Contributor Author

japhib commented Sep 8, 2022

Proposed changes: #124

@erszcz erszcz added the enhancement New feature or request label Sep 15, 2022
@erszcz
Copy link
Member

erszcz commented Sep 15, 2022

Hi, @japhib!

Thanks for your interest and contribution! I'm sorry for the slow response - with the available bandwidth I'm trying to focus more on the still unsolved problems upstream (i.e. in Gradualizer), and there's just not enough time to deal with everything here, too.

I think your idea makes perfect sense. I'll spend a bit more time thinking of it and also comparing with #120 and provide a proper answer. The latter attacks a similar problem, but on the scale of single apps, so it's possible only some rules for behaviour on the umbrella level are missing.

I'm working on enabling Gradient type-checking for a very large umbrella project for work [...]

Since you're stating upfront this is a serious application of Gradient, I'll also add a disclaimer. I don't want to detract you from using it, rather make sure you're properly informed. Gradient depends on Gradualizer and in the short term there will be some issues (false positives, maybe an odd crash once in a while) due to:

  • Gradualizer itself not being 100% complete:
    • it still cannot pass a self-check, i.e. it reports errors on its own code; some of these errors are due to limitations of the typechecker itself
    • a missing constraint solver - this means polymorphic functions aren't checked at all as of yet
  • Gradient still missing support for some Elixir-specific constructs

That being said, I hope we'll eventually sort all of the above out and we're happy to accept any contributions (code, bug reports, feature ideas). I'll be back once I go over #120 and #124 and figure out how they compare. Take care!

@japhib
Copy link
Contributor Author

japhib commented Oct 4, 2022

Just realized I never responded to this -- thanks for the response! I'm happy to work through the short term issues in Gradualizer, and I would love to contribute to the project to help improve it. I'm hoping to only gradually enable Gradient/Gradualizer checks on my company's large project, so ideally I'll be able to point out issues and/or contribute fixes to anything that comes up on whatever section of the project I'm trying to get Gradient working for. Anyways, I'm still excited about this project, and I remain convinced it's a great fit.

@erszcz
Copy link
Member

erszcz commented Oct 12, 2022

Awesome, it's really great to "hear" that! 🎉

@erszcz
Copy link
Member

erszcz commented Nov 23, 2022

It seems with #138 we have everything that you described here, @japhib. Do you think there are any other bits not provided by the PRs?

We also have a similar mechanism merged as #137, so these both together should cover practically every use case with quite a fine grained level of control.

@japhib
Copy link
Contributor Author

japhib commented Nov 24, 2022

Looks good! I’m good with closing this issue.

@japhib japhib closed this as completed Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants