Skip to content

feat(runfiles): Add static methods to Runfiles class to simplify interface#1656

Merged
aignas merged 8 commits intobazel-contrib:mainfrom
UebelAndre:mypy
Jan 10, 2024
Merged

feat(runfiles): Add static methods to Runfiles class to simplify interface#1656
aignas merged 8 commits intobazel-contrib:mainfrom
UebelAndre:mypy

Conversation

@UebelAndre
Copy link
Copy Markdown
Contributor

@UebelAndre UebelAndre commented Dec 27, 2023 •
edited by aignas
Loading

With this change we allow for more ergonomic creation of the runfiles
object and using type annotations:

from python.runfiles import Runfiles

def main() -> None:
    runfiles: Runfiles | None = Runfiles.Create()

Furthermore, the docs have been updated to describe pulling runfiles
library directly from rules_python to avoid the complexity of needing
to setup pip_parse to use this behavior.

@UebelAndre
Copy link
Copy Markdown
Contributor Author

UebelAndre commented Jan 2, 2024

cc @aignas is this one ok as well?

aignas
aignas reviewed Jan 2, 2024
View reviewed changes
Copy link
Copy Markdown
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.

I'll have a look at it later early next week.

aignas
aignas reviewed Jan 5, 2024
View reviewed changes
Copy link
Copy Markdown
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.

I personally don't like the Create method, because it smells of bad polymorphism design, but at the same time i think that allowing easy type annotations is a not win. @rickeylev, what do you think?

@UebelAndre UebelAndre requested a review from aignas January 5, 2024 18:23
aignas
aignas requested changes Jan 5, 2024
View reviewed changes
@UebelAndre UebelAndre requested a review from aignas January 6, 2024 02:59
@UebelAndre
Copy link
Copy Markdown
Contributor Author

UebelAndre commented Jan 9, 2024

If there's no blockers is this something that could be merged soon? I think it'd be good to consolidate the import paths for pypi releases and rules_python releases but that should be a separate change.

cc @aignas @rickeylev

@aignas
Copy link
Copy Markdown
Collaborator

aignas commented Jan 9, 2024

@UebelAndre, just came back from holiday.

Sorry for being pedantic, but as per the PR template todo checklist, could you please add a CHANGELOG.md and make the PR description more like a commit message? Otherwise the change looks good to me.

@UebelAndre
Copy link
Copy Markdown
Contributor Author

UebelAndre commented Jan 9, 2024 •
edited
Loading

@UebelAndre, just came back from holiday.

Sorry for being pedantic, but as per the PR template todo checklist, could you please add a CHANGELOG.md and make the PR description more like a commit message? Otherwise the change looks good to me.

What do you mean "more like a commit message"?

If you modify the description to your liking I can make sure my future PRs follow this pattern.

@UebelAndre UebelAndre requested a review from aignas January 9, 2024 01:35
@aignas
Copy link
Copy Markdown
Collaborator

aignas commented Jan 9, 2024

Condensed the message slightly and used passive voice and reworded to sound more like what is being added/changed with the commit rather than what is proposed. Thanks a lot for the improvements to rules_python!

@aignas aignas enabled auto-merge January 10, 2024 00:00
@aignas aignas added this pull request to the merge queue Jan 10, 2024
Merged via the queue into bazel-contrib:main with commit 1249567 Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aignas aignas aignas approved these changes

@rickeylev rickeylev Awaiting requested review from rickeylev rickeylev is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@UebelAndre @aignas @rickeylev