-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Support filtering mix deps output
#15009
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
base: main
Are you sure you want to change the base?
Conversation
| deps_map = Map.new(deps, fn dep -> {dep.app, dep} end) | ||
| unknown = Enum.reject(apps, &Map.has_key?(deps_map, &1)) | ||
|
|
||
| # Preserve apps order | ||
| {Enum.map(apps -- unknown, &deps_map[&1]), unknown} |
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.
| deps_map = Map.new(deps, fn dep -> {dep.app, dep} end) | |
| unknown = Enum.reject(apps, &Map.has_key?(deps_map, &1)) | |
| # Preserve apps order | |
| {Enum.map(apps -- unknown, &deps_map[&1]), unknown} | |
| apps | |
| |> Enum.map(fn app -> Enum.find(deps, & &1.app == app) end) | |
| |> Enum.reject(&is_nil/1) |
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 had something like this early on, but then realized that warning on the unknown CLI arguments would be a good idea, similar to how deps.unlock warns on deps that are not locked.
This suggestion gets rid of the unknown list. Do you find it irrelevant?
Example:
$ mix deps phoenix
warning: unknown dependency phoenix
Gives feedback that Phoenix is not a dependency of the current project or there was a typo.
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.
@rhcarvalho ah, sorry, I missed that for some reason. Let me give another try:
| deps_map = Map.new(deps, fn dep -> {dep.app, dep} end) | |
| unknown = Enum.reject(apps, &Map.has_key?(deps_map, &1)) | |
| # Preserve apps order | |
| {Enum.map(apps -- unknown, &deps_map[&1]), unknown} | |
| # Preserve `apps` order | |
| selected = Enum.flat_map(apps, fn app -> List.wrap(Enum.find(deps, & &1.app == app)) end) | |
| {selected, apps -- Enum.map(selected, & &1.app)} |
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.
Or alternatively:
| deps_map = Map.new(deps, fn dep -> {dep.app, dep} end) | |
| unknown = Enum.reject(apps, &Map.has_key?(deps_map, &1)) | |
| # Preserve apps order | |
| {Enum.map(apps -- unknown, &deps_map[&1]), unknown} | |
| # Preserve `apps` order | |
| selected = Enum.flat_map(apps, fn app -> Enum.filter(deps, & &1.app == app) end) | |
| {selected, apps -- Enum.map(selected, & &1.app)} |
The one above is slightly more efficient though.
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.
The one above is slightly more efficient though.
I didn't benchmark it. Intuitively, why would the combo of Enum.flat_map and Enum.find (or Enum.filter) be more efficient than a map lookup? Did you mean efficient in terms of memory or CPU?
My assumption was that deps >> apps. The Map.new way iterates over deps only once, then does length(apps) lookups to determine unknown andlength(apps) - length(unknown) lookups to determine selected.
Enum.filter(deps, ...) and Enum.find(deps, ...) are both O(length(deps)), right?
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.
Worst case is length(n) but in the average case find stops as soon as it finds a match. Not relevant for this code…
lib/mix/lib/mix/tasks/deps.ex
Outdated
| {deps, unknown} = | ||
| if apps == [] do | ||
| {Enum.sort_by(deps, & &1.app), []} | ||
| else | ||
| apps = Enum.map(apps, &String.to_atom/1) | ||
| filter_deps(deps, apps) | ||
| end | ||
|
|
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.
We can avoid the conditional, by abstracting that into the helper.
| {deps, unknown} = | |
| if apps == [] do | |
| {Enum.sort_by(deps, & &1.app), []} | |
| else | |
| apps = Enum.map(apps, &String.to_atom/1) | |
| filter_deps(deps, apps) | |
| end | |
| {deps, unknown} = filter_deps(deps, apps) |
and update the helper definition:
defp filter(deps, []), do: {Enum.sort_by(deps, & &1.app), []}
defp filter_deps(deps, apps) do
apps = Enum.map(apps, &String.to_atom/1)
...
endThere 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.
Thanks for the review!
While that mechanically works, don't you think it reads poorly? The first function head doesn't really filter anything, it sorts.
We'd need a more generic name like process_deps or something, which wouldn't really help understand what is going to happen without jumping several lines down to read the implementation.
I tried to add the feature with a small patch, follow the style of existing code, existing mix tasks, as closely as possible. For instance, https://github.com/elixir-lang/elixir/blob/main/lib/mix/lib/mix/tasks/deps.unlock.ex has a long defp do_run with cond for each case it can handle.
Of course, you and other maintainers have the final say on code style, and I'm happy to learn from your wisdom!
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.
Well, that's the point of abstracting it, so that you don't need to reason about the details outside of the function definition. A name like filter_and_sort_deps would suffice IMO. The only "drawback" (actually an inconsistency) is that you pass strings and it will returns atoms, but it is a helpers that is only called from one place.
ifs are tended to be avoided whenever possible if you don't compromise readability, and since you are already calling a function.
Feel free to not to take the suggestion (it's a suggestion after all :D). The code is OK as it is as well.
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.
Given the behaviour is actually different when there are no apps (i.e. there is no filtering at all), I think it is fine to have an explicit conditional (both snippets have the same number of conditionals, it is just that one does it inside as a pattern matching).
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 gave it another shot this morning, 6bc3cf7.
Went with a helper sort_or_filter. That keeps the run body shorter, and the commentary on intended behavior goes on top of the new helper.
I also added tests to explicitly check the output order (existing tests only checked for presence/absence, not order).
WDYT?
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.
Any way is fine by me, as you prefer!
lib/mix/lib/mix/tasks/deps.ex
Outdated
| This is particularly useful when you need to quickly check versions for | ||
| bug reports or similar tasks. | ||
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.
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.
My chance to ask about an official stance on code style w.r.t. extra trailing empty lines on @moduledoc and @doc :)
I never found an explanation for that and find both styles and Elixir, Phoenix and other high profile projects.
Several tasks have a @moduledoc that has that extra empty line, and that includes the current implementation of deps.ex:
elixir/lib/mix/lib/mix/tasks/deps.ex
Lines 185 to 189 in b60e424
| It supports the following options: | |
| * `--all` - lists all dependencies, regardless of specified environment | |
| """ |
What's the thought process here? Is it add extra line after a bullet list, indented block, and/or something else?!
I looked through all the files under mix/tasks/ and bullet/block seems like the pattern, except for a few files that seem to be inconsistent with that:
elixir/lib/mix/lib/mix/tasks/archive.uninstall.ex
Lines 15 to 18 in b60e424
## Command line options * `--force` - forces uninstallation without a shell prompt; primarily intended for automation """ elixir/lib/mix/lib/mix/tasks/escript.uninstall.ex
Lines 15 to 18 in b60e424
## Command line options * `--force` - forces uninstallation without a shell prompt; primarily intended for automation """ elixir/lib/mix/lib/mix/tasks/release.init.ex
Lines 10 to 18 in b60e424
@moduledoc """ Generates sample files for releases. $ mix release.init * creating rel/vm.args.eex * creating rel/remote.vm.args.eex * creating rel/env.sh.eex * creating rel/env.bat.eex """
If I inferred the pattern right, couldn't that be something that mix format takes care for us?
I'm sure missing nuance and historical context! Eager to learn.
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 don’t think j it matters much but you nailed it: no empty lines except at the end of lists.
Makes it easier to report versions of specific dependencies by allowing users (or tools) to pass dependency names as arguments to `mix deps`. Warns when a dependency is not found, similar to `mix deps.unlock`. Proposal: https://groups.google.com/g/elixir-lang-core/c/5tlLZ1yu4rQ/m/g7Z8fNWiBwAJ
b759cd4 to
6bc3cf7
Compare
Makes it easier to report versions of specific dependencies by allowing users (or tools) to pass dependency names as arguments to
mix deps.Warns when a dependency is not found, similar to
mix deps.unlock.Proposal: https://groups.google.com/g/elixir-lang-core/c/5tlLZ1yu4rQ/m/g7Z8fNWiBwAJ