Skip to content
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

Update promotion.rst to explain mode promote #11122

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

chris-armstrong
Copy link

Explain the difference between diffing and promotion and the use of (mode promote)

@maiste maiste added the docs Documentation improvements label Nov 14, 2024
@chris-armstrong
Copy link
Author

I've tried to fix the broken link, but not sure if that'll take

@chris-armstrong chris-armstrong marked this pull request as ready for review November 14, 2024 22:24
Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! I've added a few notes on how to improve the wording and accuracy a little bit.

I've also ran CI and you can see that the link does in fact work: https://dune--11122.org.readthedocs.build/en/11122/concepts/promotion.html

Diffing and Promotion
=====================

Diffing and Promotion flows relate to the output files of dune rules and comparing or storing the result in your source tree.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be good to restate that normally Dune outputs to a separate build directory and promotion is one place where this is different (thus "promotion from build tree to source tree").

@@ -76,3 +95,13 @@ repository. You can use the following workflow to update your test:
You can also use ``dune runtest --auto-promote``, which will
automatically do the promotion.

Copying generated code into the source tree
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would say "rule targets" or something of that sort. Given it is perfectly valid to promote binaries like PDFs ((run pdflatex <foo>.tex)) or images I think it's good to not exclude them.

Dune rules support a ``(mode promote)`` directive that will automatically
copy their output into your source tree. These files can then be checked in
to make it easier to browse, or to remove dependencies on the code generation
step for packaging in opam.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this paragraph is basically a duplicate of the paragraph above. Thus I'd suggest adding the reasoning why one might want to do this to the first and the how to di this to this one.

step for packaging in opam.

More information, including customising when the source is copied, can be found
in :doc:`../reference/dune/rule`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
in :doc:`../reference/dune/rule`
in :doc:`../reference/dune/rule`.

Remove duplication and standardise on "rule targets" and "rule output"

Signed-off-by: Chris Armstrong <[email protected]>
@chris-armstrong
Copy link
Author

chris-armstrong commented Nov 19, 2024

Thanks @Leonidas-from-XIV. Please see my changes above, hopefully these adequately address your comments.

Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

Thanks, I've added a few corrections but it's on the right way.

@christinerose can you please take a look at the text and check?

Diffing and Promotion
=====================

Dune writes its build output to a separate directory from your source code tree
(usually ``_build/<profile_name>``). You can use Diffing and Promotion flows
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not a profile but context. I wouldn't go into specifics beyond _build really.

--------------------------------------------------------

Dune rules support a ``(mode promote)`` directive that will automatically
copy their output into your source tree. This is more suitable for code or
Copy link
Collaborator

Choose a reason for hiding this comment

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

More suitable than what?

Signed-off-by: Christine Rose <[email protected]>
Signed-off-by: Christine Rose <[email protected]>
=======

You can use the ``(diff <file1> <file2>)`` directive in a rule to compare
the output of the rule with a copy in your source tree. It is useful when
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
the output of the rule with a copy in your source tree. It is useful when
its output with a copy in your source tree. It is useful when

Would this work to take out this one prepositional phrase? Reads a little smoother?

copy their output into your source tree. This is more suitable for code or
documentation generation flows where you want to check in the output to make
it easier to browse or to remove dependencies on the code generation step for
packaging in opam.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This last sentence is a little awkward. Does this say the same thing?

This approach suits code or documentation generation workflows where output needs to be checked to enable easier browsing or to eliminate dependencies on the code generation step during opam packaging.

If not, let’s work on it to clarify.

@christinerose
Copy link
Collaborator

Thanks, I've added a few corrections but it's on the right way.

@christinerose can you please take a look at the text and check?

I made a few minor grammatical changes and suggested a couple of sentence alteration for readability. See what you think!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants