8000 compile_pip_requirements: Fix defaulted macro args behaviour by thundergolfer · Pull Request #467 · bazel-contrib/rules_python · GitHub
[go: up one dir, main page]
Skip to content

compile_pip_requirements: Fix defaulted macro args behaviour #467

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
Merged
merged 1 commit into from
May 8, 2021
Merged

compile_pip_requirements: Fix defaulted macro args behaviour #467

merged 1 commit into from
May 8, 2021

Conversation

thundergolfer
Copy link

What is the current behavior?

  • The requirements_in arg cannot be popped from **kwargs because it is a named argument. So the arg value is always: name + ".in"
  • The requirements_txt arg is actually set by passing requirements_locked to the macro. If you pass the latter, you can set the former. If you just pass the former, the arg value is always name + ".txt", similar to the above.

The result is that the .in file must always be ${name}.in and you have to set the lock file name via an undocumented kwarg.

What is the new behavior?

Both args are given None defaults which the macro body will immediately detect and replace with the name + ".in" and name + ".txt" value if needed.

With this change the following macro instantiation now works as expected:

compile_pip_requirements(
    name = "requirements_annotated",
    requirements_in = "requirements.in",
    requirements_locked = "requirements.annotated.txt",
)

I could have done requirements_in = requirements_in or name + ".in, but I wanted to explictly check fo None and not catch anything falsey.

Another alternative is keeping the = "requirements.in" defaulting and abandoning the defaulting to the name + <ext> setup.

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:

Does this PR introduce a breaking change?

  • Yes
  • No

@google-cla google-cla bot added the cla: yes label May 8, 2021
@thundergolfer thundergolfer requested a review from alexeagle May 8, 2021 13:43
alexeagle
alexeagle approved these changes May 8, 2021
View reviewed changes
Copy link
Contributor
@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.

Thanks!!

@alexeagle alexeagle merged commit ef4d735 into master May 8, 2021
@thundergolfer thundergolfer deleted the jonathon--compile_pip_requirements-fix-may2021 branch May 9, 2021 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexeagle alexeagle alexeagle approved these changes

@brandjon brandjon Awaiting requested review from brandjon

@lberki lberki Awaiting requested review from lberki

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thundergolfer @alexeagle
0