Skip to content

Remove the rules_python_external README#391

Merged
thundergolfer merged 4 commits intomasterfrom
jonathon--remove-rules_python_external-README
Jan 10, 2021
Merged

Remove the rules_python_external README#391
thundergolfer merged 4 commits intomasterfrom
jonathon--remove-rules_python_external-README

Conversation

@thundergolfer
Copy link
Copy Markdown

PR Checklist

Please check if your PR fulfills the following requirements:

  • Does not include precompiled binaries, eg. .par files. See CONTRIBUTING.md for info
  • 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:

Description

This README was carried over during the merge of rules_python_external, and it has info that's duplicating and sometimes contradictory of the documentation in the root README. I'd favour just removing it.

If there's anything in this README that you think should remain in the repo, I can pull it out and put it in the root README under "Using the packaging rules".

Does this PR introduce a breaking change?

  • Yes
  • No

Copy link
Copy Markdown
Contributor

@hrfuller hrfuller left a comment

Choose a reason for hiding this comment

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

LGTM. Up to your discretion to add the chunks I commented on to the top level README

Comment on lines -77 to -89
# If you need to depend on the wheel dists themselves, for instance to pass them
# to some other packaging tool, you can get a handle to them with the whl_requirement macro.
filegroup(
name = "whl_files",
data = [
whl_requirement("boto3"),
]
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Might be good to document this in the packaging rules.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'll add a sub-section for it 👍

Comment on lines -51 to -61
load("@rules_python_external//:defs.bzl", "pip_install")
pip_install(
name = "py_deps",
requirements = "//:requirements.txt",
# (Optional) You can provide a python interpreter (by path):
python_interpreter = "/usr/bin/python3.8",
# (Optional) Alternatively you can provide an in-build python interpreter, that is available as a Bazel target.
# This overrides `python_interpreter`.
# Note: You need to set up the interpreter target beforehand (not shown here). Please see the `example` folder for further details.
#python_interpreter_target = "@python_interpreter//:python_bin",
)
Copy link
Copy Markdown
Contributor

@hrfuller hrfuller Dec 21, 2020

Choose a reason for hiding this comment

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

This example still seems useful to document the python_interpreter[_target] arguments.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed. It documented in the README but in words not code examples.

thundergolfer pushed a commit that referenced this pull request Jan 3, 2021
@thundergolfer thundergolfer merged commit a4a1ccf into master Jan 10, 2021
@thundergolfer thundergolfer deleted the jonathon--remove-rules_python_external-README branch January 10, 2021 12:54
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.

2 participants