Move types into source, drop Python 3.4 #96
Conversation
sethmlarson
force-pushed
the
type-hints-in-source
branch
from
3eb048e
to
b25f03e
Compare
| label = label.encode('ascii') | ||
| ulabel(label) | ||
| if not valid_label_length(label): | ||
| label_bytes = label.encode('ascii') |
There was a problem hiding this comment.
Had to make this label_bytes change because mypy doesn't like setting bytes to a variable previously typed as str. Used label_bytes throughout in this file.
sethmlarson
force-pushed
the
type-hints-in-source
branch
from
b25f03e
to
ba9e86a
Compare
|
Will move all the types into comments to adhere to Python 3.5 compatible type hinting. |
sethmlarson
force-pushed
the
type-hints-in-source
branch
2 times, most recently
from
63c9e0c
to
a49e484
Compare
sethmlarson
force-pushed
the
type-hints-in-source
branch
from
a49e484
to
56105fe
Compare
idna/core.py
Outdated
| @@ -84,7 +88,7 @@ def check_bidi(label, check_ltr=False): | |||
| raise IDNABidiError('First codepoint in label {} must be directionality L, R or AL'.format(repr(label))) | |||
|
|
|||
| valid_ending = False | |||
| number_type = False | |||
| number_type = False # type: Union[str, bool] | |||
There was a problem hiding this comment.
Could this be simplified into number_type = None # type: str by also updating the condition on L106 to if number_type is None?
(unicodedata.bidirectional may return an empty string value)
If so, perhaps fair to do that in a separate changeset if it'd be misleading or confusing to apply it at the same time as the other changes here.
There was a problem hiding this comment.
Ah, perhaps number_type = None # type: Optional[str] would be the way to go (making the nullability of the type explicit)
There was a problem hiding this comment.
Changed this to Optional[str]
| size += len(trailing_dot) | ||
| return (result, size) | ||
| return result_str, size |
There was a problem hiding this comment.
Again possibly not the correct moment to modify this within the current pull request, but I wonder if it's possible to simplify the logic around the returned string value and size variable. In particular: is size always equal to len(result_str)?
There was a problem hiding this comment.
This whole file needs a lot of work, I mentioned it in initial PR message that I'm almost certain the idna.codec module is not functional. Would recommend solving all these problems in a different PR.
|
@kjd I believe this is ready for review. Squash this on merge, the second commit is only to show that this change isn't required to merge and I can rollback if needed. |
|
Thanks! I agree that the codec implementation hasn't been looked at since the very first version and needs some attention. |
Follow-up from #94 where dropping Python 3.4 support was accepted:
python_requiresto>=3.5Also while working on this I discovered that
idna.codecis not tested and looks broken. The test suite intest_codectests the standard libraryidnacodec and not the one provided by this library. I can work on fixing that issue in a separate PR.