-
Notifications
You must be signed in to change notification settings - Fork 541
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
refactor: use bazel-skylib for creating hub repo aliases #1855
refactor: use bazel-skylib for creating hub repo aliases #1855
Conversation
With this PR the code becomes more maintainable and easier to inspect. Since bazel-skylib is already a dependency of rules_python, this is a backwards compatible change. Skipping the CHANGELOG notes because it should not be an externally visible change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
return render.alias( | ||
name = name, | ||
actual = render.select( | ||
selects, | ||
{tuple(sorted(v, key = lambda x: ("is_python" not in x, x))): k for k, v in sorted(selects.items())}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a comment here would be super helpful. It takes a bit of time to understand what this line done.
no_match_error = no_match_error, | ||
key_repr = lambda x: repr(x[0]) if len(x) == 1 else render.tuple(x), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming you're doing this to support the selects.with_or
construct from skylib?
EDIT: Okay, pretty clear from looking at the rest of the changes. Might be worth mentioning as a comment here maybe?
With this PR the code becomes more maintainable and easier to inspect.
Since bazel-skylib is already a dependency of rules_python, this is
a backwards compatible change.
Skipping the CHANGELOG notes because it should not be an externally
visible change.
Work towards #735.