-
Notifications
You must be signed in to change notification settings - Fork 443
feat: Synopsis to rule stanza, display synopses in dune show #11609
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
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
c4e5fac
to
796679e
Compare
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
4883dca
to
765d926
Compare
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
src/dune_engine/rules.mli
Outdated
val add_deps | ||
: t | ||
-> ?loc:Stdune.Loc.t | ||
-> ?synopsis:Synopsis.t option |
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 get what would the synopsis mean in this instance. Does this mean an alias can have multiple actions attached to it with different descriptions? When do we make use of this?
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.
It is used in
dune/src/dune_rules/simple_rules.ml
Line 139 in 6f5a145
Rules.Produce.Alias.add_deps |
Does this mean an alias can have multiple actions attached to it with different descriptions?
Yes, it has multiple rules with different targets attached to it.
This is essentially what allows us to print synopses in dune show aliases
dune/src/dune_rules/simple_rules.ml
Line 144 in 6f5a145
and+ targets = add_user_rule sctx ~dir ~rule ~action ~expander in |
Attaches synopsis to targets, so it can be showed in dune show targets
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 dune, there's currently two ways to attach dependencies to an alias. First, using the target of a rule:
(rule
(alias ..))
Second, by attaching it using the alias stanza:
(alias
(name ..)
(deps ..))
In both cases, we're attaching an edge from a target to the alias in that directory. I think it would be wrong to attach an edge to one form but not the other.
But more over, it doesn't even seem right to allow annotating this edge at all. In other words, you're adding the ability to add the same target to different aliases with a different description. Is that really what we're looking for? The target should have one description.
I think it would make more sense to attach the description somewhere in Dune_engine.Rule.t
. E.g see the info module for example. For loaded aliases, we don't have an explicit type, but I imagine allowing for one synopsis per alias name per directory is the right way to go.
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 5ff4fd9 and 4f204c3 I removed passing synopsis through add_deps
.
I also removed ability to add synopsis to alias
stanza.
Now, you can only add alias to rule
stanza and tests are passing as before. You can still get all aggregated synopses for alias if it attached to multiple rules.
I think it would make more sense to attach the description somewhere in Dune_engine.Rule.t.
Yes, I believe it is attached there now
Signed-off-by: Maxim Grankin <[email protected]>
…dune into alias-with-synopsis
Signed-off-by: Maxim Grankin <[email protected]>
692b097
to
a1fd6ec
Compare
@Leonidas-from-XIV I've added docs in a1fd6ec. Is there anything else I need to do from administrative PoV? For changelog entry, do I need to add this feat to Changes.m? |
No, please add a file called |
Signed-off-by: Maxim Grankin <[email protected]>
…dune into alias-with-synopsis
@Leonidas-from-XIV added changelog file in 5ebd4fc Let me know if there is anything else that needs to be done! |
Signed-off-by: Maxim Grankin <[email protected]>
05e18ab
to
95471b3
Compare
Signed-off-by: Maxim Grankin <[email protected]>
95471b3
to
4f204c3
Compare
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
2434f0a
to
5ff4fd9
Compare
Failures are the same as on main branch |
Implementation to resolve #11522
This PR makes multiple changes:
(synopsis "Doc")
torule
stanzaloc
is notnone
) viadune show aliases
dune show targets
Attach synopsis todefault
aliasHad to adjust some tests for new behaviorBumped version to 3.19Merged d274531Some examples from cram tests are: