Add type annotations to runfiles library #764
Conversation
python/runfiles/runfiles.py
Outdated
| @@ -68,16 +68,20 @@ | |||
| import os | |||
| import posixpath | |||
|
|
|||
| from typing import Callable, Dict, Optional, Tuple, Union | |||
There was a problem hiding this comment.
Do we still need the import if only using type comments and not annotations?
Will this make it backwards incompatible with older versions of Python that users may be using?
There was a problem hiding this comment.
Unfortunately yes. If we don't import this, we get errors like so:
external/rules_python/python/runfiles/runfiles.py:81: error:
Name "Optional" is not defined [name-defined]
def Create(env=None):
typing has been in the stdlib since 3.5, but for older pythons you would need typing from pypi. I'll just import this behind an if False and leave a comment explaining why
There was a problem hiding this comment.
If you're forced to import these, why not actually annotate the functions? instead of using comments? Doesn't this break backward compatibility with the long-since-EOL python2 if you need the imports? Might as well add proper annotations, right?
There was a problem hiding this comment.
I've updated this PR to not result in any runtime import changes, this is an outdated version. If we want to drop py2 there's probably a ton of cleanup than can be done, but I suspect we should do that in a more principled way than sneaking it in this PR. (In addition, the typing imports don't exist before 3.5 anyway)
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
No annotations currently
Issue Number: N/A
What is the new behavior?
Annotations
Does this PR introduce a breaking change?
No, unless we need to support very old python versions without
typingin the stdlib?Other information