Added mypy POC and attempted to fix issues #1114
Conversation
There was a problem hiding this comment.
I have a few things in the comments, but I have a few other things here:
- Could you remove supported python versions from
setup.py, if you are removing them from tests? - Could you add news file for the change? See https://anitya.readthedocs.io/en/latest/contributing.html#release-notes (I would say that this will be dev change)
- Also look at the formatting, it's failing in Travis CI.
.travis.yml
Outdated
| - stage: test | ||
| python: 3.6 | ||
| env: TOXENV=py36 | ||
| - python: 3.7 |
There was a problem hiding this comment.
Could you add mypy to .travis.yml?
There was a problem hiding this comment.
Because https://www.python.org/dev/peps/pep-0589/ - Typed Dict has been introduced in 3.8
There was a problem hiding this comment.
Ok, could you reflect this change in setup.py, it's used for creating metadata for PyPi packages about supported python versions.
| @@ -53,6 +53,16 @@ def split_by_match(regex: str, string: str) -> Tuple[str, str]: | |||
| return "", string | |||
|
|
|||
|
|
|||
| class SplitResult(TypedDict, total=True): | |||
There was a problem hiding this comment.
I'm not a fan of having multiple classes in one .py file, but this looks good. I didn't knew about the TypedDict till now.
Codecov Report
@@ Coverage Diff @@
## master #1114 +/- ##
==========================================
- Coverage 97.34% 97.19% -0.16%
==========================================
Files 64 64
Lines 4220 4241 +21
Branches 600 603 +3
==========================================
+ Hits 4108 4122 +14
- Misses 65 67 +2
- Partials 47 52 +5
Continue to review full report at Codecov.
|
AdamSaleh
force-pushed
the
anitya_mypy_poc
branch
from
1aa28c1 to
1d136d3
Compare
There was a problem hiding this comment.
I saw codecov complaining about some new lines not being tested, but it would decrease coverage only by 0.15%, so let's ignore this.
Could you do two last things?
- Add news file. See https://anitya.readthedocs.io/en/latest/contributing.html#release-notes for how. This will be a dev change
- Squash the commits together
AdamSaleh
force-pushed
the
anitya_mypy_poc
branch
from
1d136d3 to
1acf897
Compare
| @@ -0,0 +1,2 @@ | |||
| Introduced static-type checking through inclusion of mypy in tox. | |||
There was a problem hiding this comment.
I'm not sure if towncrier can process multiline news file. Let's keep this only with mypy update.
There was a problem hiding this comment.
Could you try to run towncrier --draft to test this?
There was a problem hiding this comment.
I tried it and it doesn't seems to be an issue.
|
@AdamSaleh Because the F34 is by default running on python 3.9, it would be nice to have this in as soon as possible. I deployed the new version of Anitya on staging today running F34 and it's has plenty of issues and this PR should solve them. |
|
I continued on this PR, but there is one thing missing |
|
Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
|
|
I added #1153 for Python 3.9 support, so no need to fix this here. I will fix any conflict that will arise from this. |
|
I rebased the PR to latest master. So the tests are now run against Python 3.9. |
|
Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
|
AdamSaleh
force-pushed
the
anitya_mypy_poc
branch
from
8b34930 to
245bcc4
Compare
|
Zuul encountered a syntax error while parsing its configuration in the Job tox-mypy not defined The error appears in the following project stanza: project: in "fedora-infra/anitya/.zuul.yaml@master", line 143, column 3 |
…-annotations. Removed py36 and py37.
|
Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
|
|
It looks like you rewrote my changes :-/ and I did the same by doing git pull from this PR |
|
I fixed the mypy zuul job, but it still doesn't work, although I don't understand the issue happening there |
|
I was able to reset the branch to before |
|
Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
|
Add missing requirements for tests Signed-off-by: Michal Konečný <[email protected]>
|
Build succeeded.
|
|
It looks like all the remaining issues are fixed. |
Added mypy and attempted to fix issues resulting from additional type-annotations. Removed py36.
Things I am not sure about:
The calendar comparisons: https://github.com/fedora-infra/anitya/compare/master...AdamSaleh:anitya_mypy_poc?expand=1#diff-6d66ea3411d110aa6961c53b9437d5adcecd56119de31c6a7530df1aa15314c3R491
I might go over-board with the isInstance checks, so some things are not comparable that were before. Or I have uncovered bug in the implementation? :-) If it's the former, I would like to figure out a way how to do this with mypy.
The defaults (that are not None): https://github.com/fedora-infra/anitya/compare/master...AdamSaleh:anitya_mypy_poc?expand=1#diff-7c42726a003d98fb9eab5f791a56c51d0422dde1d49020ec105961e0eadbdea3R86
Some should be easy, i.e. empty-list can be used in most places instead of None, because it already is False-y.
Some I am not sure about, like default REGEX, e.t.c.
Typed dictionary: it is really cool for mypy to shout at you when you have typo in your dictionary keys. But there already is support for dictionaries that don't contain all of the keys, and we might want to use that instead of lots of {key : None} initialization