-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
language/python: order args for virtualenv_install_with_resources
#16817
language/python: order args for virtualenv_install_with_resources
#16817
Conversation
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.
Great idea! Some more comments, none of which are blocking.
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.
In general, I like the the DSL. I left a few nits though.
The error message will be weird if someone decides to add the same name to multiple of these lists but then again, who would do that?
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
I'll merge suggestions and then adjust remaining parts after testing out logic. |
54f3367
to
e333cfc
Compare
Need another review as I refactored logic into separate method so want to make sure if this is good. I was trying out other loop logic from my review comment, but Sorbet doesn't seem capable of keeping track of types in that situation. I do think current logic is more readable than my other attempts. |
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 agree this is easier to understand than before. The error message is a bit less specific but it should be clear enough.
Add `without`, `start_with`, and `end_with` to allow basic control over the order that resources are installed so that the situations where we have to split up `virtualenv_install_with_resources` is reduced. Co-authored-by: Kevin <[email protected]> Co-authored-by: Mike McQuaid <[email protected]> Signed-off-by: Michael Cho <[email protected]>
e333cfc
to
ecb7dab
Compare
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.
Makes sense to me, thanks again @cho-m!
Add
without
,start_with
, andend_with
to allow basic control over the order that resources are installed so that the situations where we have to split upvirtualenv_install_with_resources
is reduced.brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?In
homebrew-core
, we've often have to running separate venv commands whenever there is an ordering problem or there are some extra resources that we don't want in venv.In this PR, I am looking into expanding DSL to handle some of these cases.
Examples usage can include:
jupyterlab
:awscli
I added an exception in case that resource name doesn't exist, but can remove if we want to just ignore these.
EDIT: Probably need to view test diff without whitespace as I ended up indenting existing
Language::Python::Virtualenv::Virtualenv
(https://github.com/Homebrew/brew/pull/16817/files?diff=unified&w=1)