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 type checking configuration to dsl #132

Merged
merged 2 commits into from
Apr 27, 2020
Merged

Add type checking configuration to dsl #132

merged 2 commits into from
Apr 27, 2020

Conversation

alts
Copy link
Contributor

@alts alts commented Apr 26, 2020

From the commit message:

This allows the project owner to specify the strictness of type
checks in their project. The naming of the new options draws upon
the structure of similar options in mypy[0] and the naming of the
typing errors themselves.

These two errors are very common ones that arise when trying to add
steep to an existing untyped project.

[0] https://mypy.readthedocs.io/en/stable/config_file.html

I tried adding steep to a few projects just to see what the process of adding types to an existing project was. These two typing errors came up regularly, and could be an impediment to adding typing gradually to an existing project.

@soutaro
Copy link
Owner

soutaro commented Apr 27, 2020

@alts Thanks!

  • Could you add a unit test for the new DSL syntax? I think adding a test case in project_test.rb makes sense.
  • Could you check the test failures?

@soutaro
Copy link
Owner

soutaro commented Apr 27, 2020

I took a look at the smoke test failures. The errors are because of that you implemented the method error filtering, which hasn't been implemented for months... 🙇‍♂️

I'd like to suggest to implement two major modes for type checking options, strict and moderate (or you may find better names.) In the smoke tests, we want to use the strict mode, like:

target ... do
  ...

  typing_options :strict
end

@alts
Copy link
Contributor Author

alts commented Apr 27, 2020

In that model, would moderate be the default settings?

This allows the project owner to specify the strictness of type
checks in their project. The naming of the new options draws upon
the structure of similar options in mypy[0] ang the naming of the
typing errors themselves.

These two errors are very common ones that arise when trying to add
steep to an existing untyped project.

[0] https://mypy.readthedocs.io/en/stable/config_file.html
@alts
Copy link
Contributor Author

alts commented Apr 27, 2020

The latest change introduces three strictness levels. strict, default (what existed before), and lenient. I can make moderate an alias for default, if you'd like. I also wonder about the long-term usefulness of lenient as a category rather than being accomplished through individual toggles.

I some assertions to steepfile_test.rb. I wasn't sure how to go about adding a useful test to project_test.rb. I also realize now that having the filtering of errors done in Drivers::Check probably isn't the best home. I tried moving it to Project::Target, which seems better for testability. I wasn't sure about how equivalent TypeCheckStatus#type_check_sources and Target#source_files were. It seems like there's some filtering occurring.

@soutaro
Copy link
Owner

soutaro commented Apr 27, 2020

Thanks!

:lenient sounds good! And it seems that adding assertions on steepfile_test makes sense.

I agree that having the filtering in Drivers::Check doesn't look the best, but I'm also not very sure where is the best place... So, I think we can continue with your implementation.

@soutaro soutaro merged commit 03f4c63 into soutaro:master Apr 27, 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.

2 participants