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.

Sign up for GitHub

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

Jump to bottom

Update README to say that using requirement() is optional #594

Merged
merged 2 commits into from
Jan 3, 2022
Merged

Update README to say that using requirement() is optional #594

merged 2 commits into from
Jan 3, 2022

Conversation

person142
Copy link
Contributor

@person142 person142 commented Jan 3, 2022 •
edited
Loading

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #414

Currently the documentation recommends only using requirement() to access the py_library targets generated by pip_install and pip_parse.

What is the new behavior?

Update the documentation to specify that using requirement() is optional, and that accessing the labels directly is ok.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

alexeagle
alexeagle approved these changes Jan 3, 2022
View reviewed changes
Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Awesome thank you for picking up that TODO :)

@alexeagle alexeagle merged commit 84261bc into bazelbuild:main Jan 3, 2022
aignas
aignas reviewed Jan 5, 2022
View reviewed changes
README.md
buildozer command:

```
buildozer 'substitute deps @old_([^/]+)//:pkg @new_${1}//:pkg' //...
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not work as the target list needs to be specified as //...:* for all of the targets.

Copy link
Contributor

Choose a reason for hiding this comment

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

good point @aignas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Derp, I ran this locally to test but copy pasted wrong apparently...

@hrfuller hrfuller mentioned this pull request Jan 6, 2022
Merged
12 tasks
@person142 person142 deleted the requirements-fun-docs branch January 7, 2022 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hrfuller hrfuller hrfuller left review comments

@aignas aignas aignas left review comments

@alexeagle alexeagle alexeagle approved these changes

@andyscott andyscott Awaiting requested review from andyscott

@thundergolfer thundergolfer Awaiting requested review from thundergolfer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@person142 @alexeagle @aignas @hrfuller