Remove python 2 compatibility, add type hints#80
Conversation
|
Sorry for the force-push, I've accidentally opened the PR from an earlier version of my branch than I intended. |
xuhdev
left a comment
xuhdev
left a comment
There was a problem hiding this comment.
Thanks for the PR! Please add my check to a new GitHub workflow since you added type annotations. Alternatively you can separate type annotations to a separate PR.
|
@xuhdev : Just to make sure I understand correctly: you want me to remove |
petamas
force-pushed
the
add-type-hints
branch
3 times, most recently
from
b780749 to
9c44818
Compare
| split_version[3] = "final" | ||
| split_version = list(map(int, split_version[:3])) + split_version[3:] | ||
| return tuple(split_version) | ||
| return (int(split_version[0]), int(split_version[1]), int(split_version[2]), split_version[3]) |
There was a problem hiding this comment.
the old code looks fine.
| return (int(split_version[0]), int(split_version[1]), int(split_version[2]), split_version[3]) | |
| return tuple(list(map(int, split_version[:3])) + split_version[3:]) |
There was a problem hiding this comment.
mypy doesn't understand the old code, that's why I changed it. (Also, I think it's much more readable after the change, but that's just my opinion.)
editorconfig/versiontools.py
Outdated
| """ | ||
|
|
||
| import re | ||
| from typing import Optional, Tuple |
There was a problem hiding this comment.
Optional is still needed on 3.9, but Tuple can be written as tuple.
There was a problem hiding this comment.
You're absolutely right, I was targeting 3.8 when I wrote it, but with 3.9, it's unnecessary . I'll update all Tuples to tuple and Lists to list.
|
@greut: I've addressed the I also tried the old version of the It successfully figures out that |
| test: | ||
| name: Static type checking | ||
| runs-on: ubuntu-latest | ||
| container: python:3.13-alpine3.21 |
There was a problem hiding this comment.
Not sure if we want to use alpine3.21 here, renovate bots won't bump 3.21. Maybe just alpine or use setup-python?
There was a problem hiding this comment.
That part was copied verbatim from the other workflow (runtime.yml).
There was a problem hiding this comment.
I'm happy to change it of course, just wanted to point out that sticking to a specific alpine version was not my own decision.
There was a problem hiding this comment.
Ok, it's fine then. We can tackle it separately.
|
ping @greut , does this look good to you :) |
|
@greut, does this look OK to you? I'd appreciate if this could be merged. |
|
Thanks for the contribution! I merged for now, @greut is always welcome to comment and we can do a followup. |
|
@xuhdev : Thanks! Question: what's the timeline on the next release of the package that includes my changes? |
|
We can make a bug fix release on demand. Let me know what you think; feel free to create a patch version bump PR. I can tag it :) |
3 participants
Resolves #78
This is my first time using Travis, so I hope it'll work as I think it works.