Skip to content

Improve README documentation of pip_install rule [tiny] #501

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

Merged
merged 3 commits into from
Jul 5, 2021

Conversation

thundergolfer
Copy link

@thundergolfer thundergolfer commented Jul 1, 2021

On re-reading this part of the README, I thought the "central repo" and "knows about" wording could be improved.

🖱 Click me to preview README changes

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:

Does this PR introduce a breaking change?

  • Yes
  • No

@thundergolfer thundergolfer requested a review from andyscott as a code owner July 1, 2021 04:18
@google-cla google-cla bot added the cla: yes label Jul 1, 2021
@thundergolfer thundergolfer requested review from hrfuller and removed request for andyscott July 1, 2021 04:19
target in the appropriate wheel repo.

### Importing `pip` dependencies
### Installing `pip` dependencies
Copy link
Author

@thundergolfer thundergolfer Jul 1, 2021

Choose a reason for hiding this comment

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

"Installing" is more correct then "Importing" I think, as it matches pip install. Import is what happens in a module.

`BUILD` files to translate a pip package name into the label of a `py_library`
Usage of the packaging rules involves two main steps.

1. [Installing `pip` dependencies](#installing-pip-dependencies)
Copy link
Author

Choose a reason for hiding this comment

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

Change related to issue #406. Specifically #406 (comment)

pip_install(
name = "my_deps",
requirements = "//path/to:requirements.txt",
)
```

Note that since pip is executed at WORKSPACE-evaluation time, Bazel has no
Note that since `pip_install` is a repository rule and therefore executes pip at WORKSPACE-evaluation time, Bazel has no
Copy link
Author

Choose a reason for hiding this comment

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

Passive voice to active voice, and being explicit about what rule is executing pip.

downloaded wheel files, and individual external repos for each wheel's extracted
contents. Users only need to interact with the central external repo; the wheel repos
are essentially an implementation detail. The central external repo provides a
`WORKSPACE` macro to create the wheel repos, as well as a function, `requirement()`, for use in
Copy link
Author

Choose a reason for hiding this comment

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

Naming the function being provided.

@UebelAndre
Copy link
Contributor

@thundergolfer
Copy link
Author

@UebelAndre it's still supported in the rules so it should be documented somewhere. It is distracting but also easiest to maintain in the main README.

@UebelAndre
Copy link
Contributor

@UebelAndre it's still supported in the rules so it should be documented somewhere. It is distracting but also easiest to maintain in the main README.

Then why call it "legacy"? It sounds like something that is no longer being developed and I'd generally venture to say "deprecated" in that case. I think because there are now releases, users should be looking at the rev of the release they use for the docs they're interested in. So it's not necessary to maintain docs for something that's "legacy".

@thundergolfer
Copy link
Author

@hrfuller and @alexeagle (who I think on vacation) what do you think RE @UebelAndre's suggestion?

@thundergolfer
Copy link
Author

@UebelAndre I'll merge this as it progresses in the right direction, but will keep open the issue of whether pip_import should be removed from the README. If maintainers agree to remove it, we can create a follow up PR.

@thundergolfer thundergolfer merged commit 9256757 into main Jul 5, 2021
@thundergolfer thundergolfer deleted the jonathon--improve-README-code-cmmnt-june2021 branch July 5, 2021 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants