Incorrect check for NoneType
#95
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #95 +/- ##
==========================================
- Coverage 93.34% 93.04% -0.30%
==========================================
Files 32 32
Lines 3364 3364
Branches 619 695 +76
==========================================
- Hits 3140 3130 -10
- Misses 149 159 +10
Partials 75 75 ☔ View full report in Codecov by Sentry. |
…r python 3.7 in 1.6.0. Cannot support 3.12 without dropping support for 3.7.
|
I think the codecov failure is due to extra lines being added by black reformatting. Would appreciate if we can merge despite that failure. |
jdidion
changed the title
type is not necessaryNoneType
There was a problem hiding this comment.
Thank-you for fixing all this. Nonetheless, there's a lot of other fixes that are not related to this PR, though nice to have in general (again thank-you!). I'd much prefer to see separate PRs for each (e.g. dropping 3.7 support, adding 3.11 support, reformatting TOML, fix black formatting, etc). If that's too onerous, I would want to see individual commits for the above, to make it clear the intent of each set of changes. Is it too onerous to do either at this point?
| @@ -226,7 +228,7 @@ def dict_parse(dict_string: str) -> Dict[Any, Any]: | |||
| return types.make_enum_parser(type_) | |||
| elif types.is_constructible_from_str(type_): | |||
| return functools.partial(type_) | |||
| elif isinstance(type_, type(type(None))): | |||
| elif type_ is type(None): # noqa: E721 | |||
There was a problem hiding this comment.
@jdidion following up on your question from Slack, I think this is the right approach.
as far as I'm aware, flake8 guards against type(x) == int and the like. We're doing equality checks against types above (e.g. if type_ == bool) and flake8 doesn't complain, I'm guessing because we don't call type() in the same line.
If you wanted to avoid the noqa, you could declare NoneType = type(None) above (or in fgpyo.types and import it), then replace this line with elif type_ is NoneType. But this feels like an edge case in flake8, so I think what you have is fine if you're fine with it
Fixes the check for
NoneType.type(type(None))returns<class type>, and soisinstance(type_, type(type(None)))always returnsTrue. Instead, we wanttype_ is type(None).I found that one file
fgpyo.fasta.builderis formatted differently with black 20.8b1 (which I had installed locally) vs 23.3.0 (the latest version, which is installed during CI), causing a build failure, so I just updated the minimum to the latest version.I added support for 3.11. We cannot add support for 3.12 without dropping support for 3.7 due to the python versions supported by different versions of Poetry. Thus, I explicitly limited the supported versions for fgpyo in pyproject.toml to 3.7-3.11.
I updated the formatting of pyproject.toml based on my IDE's built-in TOML formatter.
I have removed
3.7from the CI test matrix because it is failing with some gnarly mypy errors that are not there in more recent versions (in which support for 3.7 has been dropped). I advocate dropping support for 3.7 as it is EOL. That would allow us to get rid of dependencies and code ininspect.pyandtypes.pythat are only there to maintain 3.7 compatibility.Closes #94