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): Add static methods to Runfiles class to simplify interface #1656

Merged
merged 8 commits into from
Jan 10, 2024

Conversation

UebelAndre
Copy link
Contributor

@UebelAndre UebelAndre commented Dec 27, 2023

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
Contributor Author

cc @aignas is this one ok as well?

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.

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

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.

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?

python/runfiles/README.md Show resolved Hide resolved
@UebelAndre UebelAndre requested a review from aignas January 5, 2024 18:23
python/runfiles/README.md Outdated Show resolved Hide resolved
@UebelAndre UebelAndre requested a review from aignas January 6, 2024 02:59
@UebelAndre
Copy link
Contributor Author

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
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
Contributor Author

UebelAndre 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.

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
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 bazelbuild:main with commit 1249567 Jan 10, 2024
4 checks passed
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.

3 participants