Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(runfiles): Added type hints to @rules_python//python/runfiles #1654

Merged
merged 5 commits into from Dec 27, 2023

Conversation

UebelAndre
Copy link
Contributor

@UebelAndre UebelAndre commented Dec 24, 2023

This change adds mypy types directly to the rules_python.python.runfiles library as well as regression testing to CI.

@UebelAndre UebelAndre changed the title Added type hints to @rules_python//python/runfiles feat(tools): Added type hints to @rules_python//python/runfiles Dec 24, 2023
@UebelAndre UebelAndre changed the title feat(tools): Added type hints to @rules_python//python/runfiles feat(runfiles): Added type hints to @rules_python//python/runfiles Dec 24, 2023
@UebelAndre UebelAndre marked this pull request as ready for review December 24, 2023 04:26
python/runfiles/runfiles.py Show resolved Hide resolved
python/runfiles/runfiles.py Show resolved Hide resolved
.github/workflows/mypy.yaml Outdated Show resolved Hide resolved
tests/runfiles/runfiles_test.py Outdated Show resolved Hide resolved
python/runfiles/runfiles.py Outdated Show resolved Hide resolved
python/runfiles/runfiles.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me this looks good, but I'd like to wait for @rickeylev to have a look at this since this would constrain the runfiles library to Python 3.7 and I don't know if this is something we want to do now.

EDIT: According to https://devguide.python.org/versions/, it is already EOL, so this concern may not be applicable and we can move forward to the better future with type annotations.

@UebelAndre
Copy link
Contributor Author

UebelAndre commented Dec 26, 2023

For me this looks good, but I'd like to wait for @rickeylev to have a look at this since this would constrain the runfiles library to Python 3.7 and I don't know if this is something we want to do now.

EDIT: According to https://devguide.python.org/versions/, it is already EOL, so this concern may not be applicable and we can move forward to the better future with type annotations.

For what it’s worth, 3.7 was chosen because it’s EOL and I thought it would be a sufficiently low bar.

UebelAndre and others added 2 commits December 25, 2023 22:39
Co-authored-by: Ignas Anikevicius <240938+aignas@users.noreply.github.com>
Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@aignas aignas added this pull request to the merge queue Dec 27, 2023
Merged via the queue into bazelbuild:main with commit 4c2d7d9 Dec 27, 2023
4 checks passed
@UebelAndre UebelAndre deleted the mypy branch December 27, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants