feat: Support use of TYPE_CHECKING with simple boolean logic #142
Conversation
|
It seems the 3.10.7 test workflow failed because of a network issue. Perhaps it can be retried. |
Codecov Report
@@ Coverage Diff @@
## main #142 +/- ##
=======================================
- Coverage 98.7% 98.1% -0.6%
=======================================
Files 3 3
Lines 472 487 +15
=======================================
+ Hits 466 478 +12
- Misses 6 9 +3
|
|
Sorry @steverice, I think I saw the test failed when you posted this and assumed you'd make edits, then I missed your comment and forgot about it 🙇 This looks good to me. Would you mind rebasing the PR and I'll take one final look at it before merging and releasing 👍 |
For various reasons, modules may be conditioned on multiple criteria that determine whether they should be imported.
For example, if we have an module that should be imported during testing or type checking, we may do something like:
```python
if TYPE_CHECKING or environment == 'test':
import foobar
```
Currently, the linter will flag the `foobar` import as needing to be moved into an `if TYPE_CHECKING` block in this scenario.
This gives us two options, each of which is less than ideal.
We could import the module twice, i.e.
```python
if TYPE_CHECKING:
import foobar
elif environment == 'test':
import foobar
```
or use a `# noqa` comment and lose the safety of the check when the code changes.
This commit adds some logic to handle these types of simple conditionals and determine if they are equivalent to `if TYPE_CHECKING:` for the purposes of this linter.
In our simple example, we should be able to determine that `if TYPE_CHECKING or environment == 'test'` will always result in the module being imported if `TYPE_CHECKING` is True.
Similarly, we should be able to determine that `if TYPE_CHECKING and environment == 'test'` is _not_ sufficient, since we have no guarantees about the value of `environment`.
steverice
force-pushed
the
additional-boolean-logic
branch
from
321c467
to
acdbe29
Compare
|
If you're up for it, that would be amazing 👏 |
|
@sondrelg #144 and #145 contain the fixes for the test coverage. Let's merge those first and then I'll rebase this. Codecov is complaining on all three of these. Hopefully it's clear that this is due to the existing coverage issues, as a commit that only adds new tests can't possibly decrease coverage 😄 |
|
Thanks again 👍 Don't think you need to rebase this, I'll merge as is and release a new minor version shortly |
For various reasons, modules may be conditioned on multiple criteria that determine whether they should be imported. For example, if we have an module that should be imported during testing or type checking, we may do something like:
Currently, the linter will flag the
foobarimport as needing to be moved into anif TYPE_CHECKINGblock in this scenario. This gives us two options, each of which is less than ideal. We could import the module twice, i.e.or use a
# noqacomment and lose the safety of the check when the code changes.This commit adds some logic to handle these types of simple conditionals and determine if they are equivalent to
if TYPE_CHECKING:for the purposes of this linter. In our simple example, we should be able to determine thatif TYPE_CHECKING or environment == 'test'will always result in the module being imported ifTYPE_CHECKINGis True. Similarly, we should be able to determine thatif TYPE_CHECKING and environment == 'test'is not sufficient, since we have no guarantees about the value ofenvironment.