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

Add doc for config file #6392

Merged
merged 26 commits into from
Nov 20, 2022
Merged

Add doc for config file #6392

merged 26 commits into from
Nov 20, 2022

Conversation

nojb
Copy link
Collaborator

@nojb nojb commented Nov 6, 2022

Fixes #5493

Moreover, the "man" page for dune-config(5) is terribly out-of-date and does not seem very pratical to have the information in two places (the manual and the man page). Since we don't document any other Dune file format in man page format, shall we get rid of the dune-config(5) man page?

@nojb nojb requested a review from christinerose as a code owner November 6, 2022 09:53
@rgrinberg rgrinberg added the docs Documentation improvements label Nov 8, 2022
Copy link
Contributor

@christinerose christinerose left a comment

Choose a reason for hiding this comment

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

Mostly grammar & formatting.
I’ve questions about the subheadings. If they’re commands or files, like config, it is possible to show them in monospace in the heading? If so, that’s best for clarify. If not, perhaps consider capitalizing them, like Concurrency, etc.

@nojb
Copy link
Collaborator Author

nojb commented Nov 9, 2022

Mostly grammar & formatting.

Thanks. I committed all suggestions.

I’ve questions about the subheadings. If they’re commands or files, like config, it is possible to show them in monospace in the heading? If so, that’s best for clarify. If not, perhaps consider capitalizing them, like Concurrency, etc.

I switched them to code style.

@nojb
Copy link
Collaborator Author

nojb commented Nov 15, 2022

@christinerose could you please give a formal approval if you are happy with the current state of the PR? Thanks!

@Alizter
Copy link
Collaborator

Alizter commented Nov 15, 2022

I don't think I like the location of the configuration documentation here. I think it would be better for it to come near the end of dune-files.rst since this otherwise it is the first thing that will be seen in the stanza reference manual. It is an opt-in and not essential to most users. So it should be:

  • dune project
  • dune
  • dune workspace
  • dune config

with some colors to help differentiate programs.

- ``short`` print one line per command being executed, with the binary name on
the left and the reason it is being executed for on the right.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about:

prints a line for each program executed with the binary name on the left and the targets of the action on the right.

- ``progress``, Dune shows and updates a status line as build goals are being
completed (this is the default).

- ``verbose`` prints the full command lines of programs being executed by Dune,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- ``verbose`` prints the full command lines of programs being executed by Dune,
- ``verbose`` prints the full command lines of programs executed by Dune,

(looking to tighten syntax a little, but it's fine the way it is if you prefer)

with some colors to help differentiate programs.

- ``short`` print one line per command being executed, with the binary name on
the left and the reason it is being executed for on the right.
Copy link
Contributor

Choose a reason for hiding this comment

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

Or...

prints a line for each program executed with the binary name on the left and the action targets on the right

???

It's also fine the way it is if you prefer. Just looking to tighten syntax where possible and minimize passive voice. :)

@christinerose
Copy link
Contributor

@christinerose could you please give a formal approval if you are happy with the current state of the PR? Thanks!

So sorry for the delay! Approved now.

@nojb
Copy link
Collaborator Author

nojb commented Nov 19, 2022

I don't think I like the location of the configuration documentation here. I think it would be better for it to come near the end of dune-files.rst since this otherwise it is the first thing that will be seen in the stanza reference manual. It is an opt-in and not essential to most users. So it should be:

Thanks for the detailed comments @Alizter! I implemented all of your recommendations. Do you mind giving a formal approval (if you are happy with the state of the PR)?

@Alizter Alizter self-requested a review November 20, 2022 00:41
Copy link
Collaborator

@Alizter Alizter left a comment

Choose a reason for hiding this comment

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

Looking good. Some more comments.

config
======

This file is used to set global configuration of Dune (applicable across
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
This file is used to set global configuration of Dune (applicable across
This file is used to set the global configuration of Dune (applicable across

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

nojb and others added 15 commits November 20, 2022 08:37
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Co-authored-by: Christine Rose <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
nojb added 10 commits November 20, 2022 08:37
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
@nojb
Copy link
Collaborator Author

nojb commented Nov 20, 2022

Looking good. Some more comments.

Thanks @Alizter! I amended as per your comments and will merge once CI passes.

@nojb nojb merged commit 17a66e0 into ocaml:main Nov 20, 2022
@nojb nojb deleted the doc_config branch November 20, 2022 08:31
Comment on lines +2210 to +2226
concurrency
-----------

Maximum number of concurrent jobs Dune is allowed to have.

.. code:: scheme

(concurrency <setting>)

where ``<setting>`` is one of:

- ``auto``, auto-detect maximum number of cores. This is the default value.

- ``<number>``, a positive integer specifying the maximum number of jobs Dune
may use simultaneously.

.. _terminal-persistence:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This appears to be made up. We should have called this jobs.

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.

Changlog feedback, missing docs (re: action-stdout-on-success)
4 participants