-
Notifications
You must be signed in to change notification settings - Fork 994
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
removing more of dependency_has_directory feature flag #10252
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.
I'm a little unclear on the method name being used for grouped dependencies, but I don't see that as being the core of this change. I do think we'd really benefit from making it clearer, but want to unblock this so you can keep working 😄
@@ -132,7 +121,7 @@ def handled_dependencies | |||
|
|||
# rubocop:disable Performance/Sum | |||
sig { returns(T::Set[String]) } | |||
def handled_group_dependencies | |||
def handled_dependencies_all_directories |
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.
🤔 _all_directories
reads as being very open to interpretation. I could see this signaling:
- That the dependencies are mapped to all directories (a product-like result)
- That the dependencies are returned agnostic of their directory 👈
- That the dependencies are combined with their directories (and/or all directories)
From context, I suspect it's the one with 👈 next to it.
Are directories
important here? If they are, I wonder if there's another signal we could use to make it unambiguous.
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.
Naming is hard!
In #9610 what I was doing was rename all the directory-contextually-aware code to have a _current_directory
suffix, would that be clearer? Then we can leave this as handled_dependencies
or even all_handled_dependencies
?
What are you trying to accomplish?
This reverts the addition of
add_handled_group_dependencies
in #9938 because it's functionally equivalent to the existingadd_handled_dependencies
.PR #8963 added support for multi-dir handling of dependencies. The way our update works, and how DependencySnapshot keeps track of the directory can be described with this pseudocode:
So anytime
add_handled_dependencies
is called, it uses the@current_directory
which was set earlier. It has context of which directory is currently being updated.add_handled_group_dependencies
does the same thing differently by passing in an array of hashes which has the dependency name and the current directory, but it results in the same outcome.To further illustrate that, I've kept the existing tests and replaced the calls to
add_handled_group_dependencies
with calls toadd_handled_dependencies
(and setting thecurrent_directory
) which still passes.Additionally I have renamed
handled_group_dependencies
tohandled_dependencies_all_directories
to reflect what it returns.How will you know you've accomplished your goal?
Again, there should be no functional difference here.
Checklist