Skip to content

feature: --all option for dune show targets#8167

Closed
Alizter wants to merge 1 commit intoocaml:mainfrom
Alizter:ps/branch/feature____all_option_for_dune_show_targets
Closed

feature: --all option for dune show targets#8167
Alizter wants to merge 1 commit intoocaml:mainfrom
Alizter:ps/branch/feature____all_option_for_dune_show_targets

Conversation

@Alizter
Copy link
Copy Markdown
Collaborator

@Alizter Alizter commented Jul 10, 2023

We change dune show targets to omit source files by default and add an --all option which will show all targets including those that come from the source tree.

Addresses wish in #7784.

  • changelog
  • tests

We change `dune show targets` to omit source files by default and add an
`--all` option which will show all targets including those that come
from the source tree.

Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter Alizter force-pushed the ps/branch/feature____all_option_for_dune_show_targets branch from b25e557 to 481139c Compare July 10, 2023 15:50
@Alizter Alizter requested a review from rgrinberg July 10, 2023 15:50
@Alizter Alizter marked this pull request as ready for review July 10, 2023 15:51
@snowleopard
Copy link
Copy Markdown
Collaborator

While this seems like an improvement, I find it very unintuitive that dune show targets --all shows targets and sources.

I guess I still prefer dune show files command, with some filters on top.

Comment on lines +157 to +158
"Print all targets, including those that exist in the source \
tree.")
Copy link
Copy Markdown
Collaborator

@snowleopard snowleopard Jul 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"All targets including those that exist in the source tree" doesn't make much sense in either of the two worlds:

  • If one considers sources to be targets, then the "including ..." part seems tautological/unnecessary, just like "including targets whose name starts with 'a'" would be unnecessary.
  • If one doesn't consider sources to be targets, then the "print all targets" makes no sense.

@snowleopard
Copy link
Copy Markdown
Collaborator

snowleopard commented Jul 10, 2023

I guess I still prefer dune show files command, with some filters on top.

There just seems to be no way to make things non-confusing when you use "targets" or "sources" as the starting point while taking about a generic concept (such as files). Neither targets nor sources are more general than the the other. Files are!

@Alizter
Copy link
Copy Markdown
Collaborator Author

Alizter commented Jul 10, 2023

@snowleopard I think I might have a different definition of "target" to the one you are thinking about. For me a target is anything that "dune build" accepts minus aliases (that's another story).

I would be happy to remove the --all argument completely if we agree that inspecting the source is uninteresting. However your suggestion of dune show files doesn't make sense to me since it is just ls at this point?

@snowleopard
Copy link
Copy Markdown
Collaborator

snowleopard commented Jul 10, 2023

For me a target is anything that "dune build" accepts minus aliases (that's another story).

This definition is pretty inconsistent with, say, what we call targets in actions. Rules have deps (sometimes sources, sometimes other targets) and targets (some of which might actually be promoted and committed as sources).

You are designing a UX feature and it's worth thinking about the least confusing way of fitting it with all other Dune concepts.

However your suggestion of dune show files doesn't make sense to me since it is just ls at this point?

It's not ls, as I keep saying. There are files that Dune ignores, and they will not appear in the output. Also, there are directory targets, which shouldn't appear in dune show files either (but can appear in dune show dirs).

UX is hard! I think neither the current state of things, nor this PR in the current form, make for a good, non-confusing UX story.

@snowleopard
Copy link
Copy Markdown
Collaborator

snowleopard commented Jul 10, 2023

UX is hard! I think neither the current state of things, nor this PR in the current form, make for a good, non-confusing UX story.

But also, just to reiterate, I'm far from being a UX expert. Sometimes it's worth talking to users. You can try to talk to them to find out how they think about the sources/targets distinction. We, developers, often have a very skewed view on things.

@Alizter Alizter marked this pull request as draft July 12, 2023 14:02
@Alizter
Copy link
Copy Markdown
Collaborator Author

Alizter commented Jul 12, 2023

OK @snowleopard thanks for your valuable feedback. I think I will draft this for now and think about it more when I have some time.

@snowleopard
Copy link
Copy Markdown
Collaborator

OK @snowleopard thanks for your valuable feedback. I think I will draft this for now and think about it more when I have some time.

Happy to brainstorm it at some point to try to figure out a way forward!

@rgrinberg rgrinberg closed this Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants