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

Render short option with arg. #9043

Merged
merged 14 commits into from
Jul 14, 2023

Conversation

philderbeast
Copy link
Collaborator

@philderbeast philderbeast commented Jun 19, 2023

See #8785.

I'm not sure a changelog entry is needed but code snippets of the command --help output may need to be updated in the docs.

<  -v, --verbose[=n]              Control verbosity (n is 0--3, default
>  -v[n], --verbose[=n]           Control verbosity (n is 0--3, default
<  -w, --with-compiler=PATH       give the path to a particular compiler
>  -wPATH, --with-compiler=PATH   give the path to a particular compiler
<  -O, --enable-optimization[=n]  Build with optimization (n is 0--2, default is
>  -O[n], --enable-optimization[=n]
>                                 Build with optimization (n is 0--2, default is
<  -f, --flags=FLAGS              Force values for the given flags in Cabal
>  -fFLAGS, --flags=FLAGS         Force values for the given flags in Cabal
<  -c, --constraint=CONSTRAINT    Specify constraints on a package (version,
>  -cCONSTRAINT, --constraint=CONSTRAINT
>                                 Specify constraints on a package (version,
<  -j, --jobs[=NUM]               Run NUM jobs simultaneously (or '$ncpus' if no
>  -j[N], --jobs[=NUM]            Run NUM jobs simultaneously (or '$ncpus' if no

@philderbeast
Copy link
Collaborator Author

I stopped short making changes like this:

<  -v, --verbose[=n]              Control verbosity (n is 0--3, default
>  -v[L], --verbose[=LEVEL]       Control verbosity (LEVEL is 0--3, default

@ulysses4ever
Copy link
Collaborator

Looks great, thank you! One question:

 -j[N], --jobs[=NUM]            Run NUM jobs simultaneously (or '$ncpus' if no

It's a bit strange to have N in one place and NUM in other. And then, the text only mentions NUM.


It does need a changelog because it's a user-visible change.

@philderbeast
Copy link
Collaborator Author

Thanks @ulysses4ever for the question using N for abbreviating NUM. I'm not doing that anymore.

-jNUM, --jobs[=NUM]

@philderbeast
Copy link
Collaborator Author

In going through the docs I'm seeing inconsistencies in the short option handling spaces, some options allow it and some don't (those that have a default value?).

$ cabal run cabal build Cabal-syntax -- -v3
Warning: this is a debug build of cabal-install with assertions enabled.
Searching for curl in path.
Found curl at /usr/bin/curl
Searching for powershell in path.
...
$ cabal run cabal build Cabal-syntax -- -v 3
Resolving dependencies...
Warning: this is a debug build of cabal-install with assertions enabled.
...
Error: cabal: Unknown target '3'.
There is no component '3'.
The project has no package '3'.
$ cabal run cabal build Cabal-syntax -- -wghc-8.2.2
Warning: this is a debug build of cabal-install with assertions enabled.
Resolving dependencies...
Error: cabal: Could not resolve dependencies:
...
[__2] fail (backjumping, conflict set: Cabal, base, time)
After searching the rest of the dependency tree exhaustively, these were the
goals I've had most trouble fulfilling: time, Cabal, base
$ cabal run cabal build Cabal-syntax -- -w ghc-8.2.2
Warning: this is a debug build of cabal-install with assertions enabled.
Resolving dependencies...
Error: cabal: Could not resolve dependencies:
...
[__2] fail (backjumping, conflict set: Cabal, base, time)
After searching the rest of the dependency tree exhaustively, these were the
goals I've had most trouble fulfilling: time, Cabal, base

@philderbeast
Copy link
Collaborator Author

philderbeast commented Jun 19, 2023

It would be nice if the short options could parse the = like the long options do:

<  -v, --verbose[=n]              Control verbosity (n is 0--3, default
>  -vn, --verbose[=n]             Control verbosity (n is 0--3, default
<  -w, --with-compiler=PATH       give the path to a particular compiler
>  -w PATH or -wPATH (but not -w=PATH), --with-compiler=PATH
>                                 give the path to a particular compiler
<  -O, --enable-optimization[=n]  Build with optimization (n is 0--2, default is
>  -On, --enable-optimization[=n]
>                                 Build with optimization (n is 0--2, default is
<  -f, --flags=FLAGS              Force values for the given flags in Cabal
>  -f FLAGS or -fFLAGS (but not -f=FLAGS), --flags=FLAGS
>                                 Force values for the given flags in Cabal
<  -c, --constraint=CONSTRAINT    Specify constraints on a package (version,
>  -c CONSTRAINT or -cCONSTRAINT (but not -c=CONSTRAINT), --constraint=CONSTRAINT
>                                 Specify constraints on a package (version,
<  -j, --jobs[=NUM]               Run NUM jobs simultaneously (or '$ncpus' if no
>  -jNUM (but not -j=NUM), --jobs[=NUM]
>                                 Run NUM jobs simultaneously (or '$ncpus' if no

@philderbeast
Copy link
Collaborator Author

As it is now (without the buts):

<  -v, --verbose[=n]              Control verbosity (n is 0--3, default
>  -v[n], --verbose[=n]           Control verbosity (n is 0--3, default
<  -w, --with-compiler=PATH       give the path to a particular compiler
>  -w PATH or -wPATH, --with-compiler=PATH
>                                 give the path to a particular compiler
<  -O, --enable-optimization[=n]  Build with optimization (n is 0--2, default is
>  -O[n], --enable-optimization[=n]
>                                 Build with optimization (n is 0--2, default is
<  -f, --flags=FLAGS              Force values for the given flags in Cabal
>  -f FLAGS or -fFLAGS, --flags=FLAGS
>                                 Force values for the given flags in Cabal
<  -c, --constraint=CONSTRAINT    Specify constraints on a package (version,
>  -c CONSTRAINT or -cCONSTRAINT, --constraint=CONSTRAINT
>                                 Specify constraints on a package (version,
<  -j, --jobs[=NUM]               Run NUM jobs simultaneously (or '$ncpus' if no
>  -j[NUM], --jobs[=NUM]          Run NUM jobs simultaneously (or '$ncpus' if no

@ulysses4ever
Copy link
Collaborator

Great job here! How about a changelog?

@philderbeast
Copy link
Collaborator Author

There's a lot of places in the *.rst docs where we could make changes (grep -\S+= 197 results in 7 files) but I don't know what is the best option (pun intended). Should we:

  1. Delete the short option from the docs, relying on the command line to show it?
  2. Show the short option "as rendered" in the docs where they are now and add short options that are missing?

Regardless of choosing 1 or 2, we might also add a new section to the docs describing how short options and long options are placed with regard their arguments, space or no space, equals permitted or not?

@ulysses4ever
Copy link
Collaborator

@philderbeast I'm sorry but I can't quite get 1 and 2 and difference between. An example could help. Also, the question you're posing sounds like something that other projects like cabal have solved before. Could you look what GHC / stack / rustc / cargo do and pick something more popular?

As for a new section -- I'm sure everyone will be happy to have documentation extended.

@philderbeast
Copy link
Collaborator Author

philderbeast commented Jul 10, 2023

@ulysses4ever I went through and updated what was there already in the docs keeping it in line with the --help output.

@@ -1536,7 +1539,7 @@ Advanced global configuration options
-------------------------------------

.. cfg-field:: write-ghc-environment-files: always, never, or ghc8.4.4+
--write-ghc-environment-files=policy
--write-ghc-environment-files=always\|never\|ghc8.4.4+
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why it's not simply --write-ghc-environment-files=POLICY, i.e. why inline the options that are listed just one line above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you want me to update the command output to use POLICY? This is what we have right now:

--write-ghc-environment-files=always|never|ghc8.4.4+
                                Whether to create a .ghc.environment file
                                after a successful build (v2-build only)

@@ -377,7 +377,7 @@ folder. By default the latest version of the package is downloaded, you can
ask for a spefic one by adding version numbers
(``cabal get random-1.0.0.1``).

.. option:: -s, --source-repository[=head|this|...]]
.. option:: -s[[head|this|...]], --source-repository[=[head|this|...]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it have to have double braces in -s[[head|this|...]]? I'd think that one pair is enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I grabbed what was rendered as-is from the console.

.. option:: --v1-freeze-file
.. option:: --freeze-file
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about this change. I think cabal outdated does work only with v1-things.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is what we have rendered:

ghci> :main outdated --help
Check for outdated dependencies.

Usage: <interactive> outdated [FLAGS] [PACKAGES]

Checks for outdated dependencies in the package description file or freeze
file

Flags for outdated:
 -h, --help                     Show this help text
 --project-dir=DIR              Set the path of the project directory
 --project-file=FILE            Set the path of the cabal.project file
                                (relative to the project directory when
                                relative)
 -v[n], --verbose[=n]           Control verbosity (n is 0--3, default
                                verbosity level is 1)
 --freeze-file                  Act on the freeze file
 --v2-freeze-file               Act on the new-style freeze file (default:
                                cabal.project.freeze)
 --simple-output                Only print names of outdated dependencies, one
                                per line
 --exit-code                    Exit with non-zero when there are outdated
                                dependencies
 -q, --quiet                    Don't print any output. Implies '--exit-code'
                                and '-v0'
 --ignore=PKGS                  Packages to ignore
 --minor[=PKGS]                 Ignore major version bumps for these packages


Both of these commands do the same thing as the above, but only expose ``base``,
``vector``, and the ``vector`` package's transitive dependencies even if the user
is in a project context.

::

$ cabal repl --ignore-project --build-depends "vector >= 0.12 && < 0.13"
$ cabal repl --project='' --build-depends "vector >= 0.12 && < 0.13"
$ cabal repl --ignore-project --build-depends="vector >= 0.12 && < 0.13"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it not work with just a space? I thought it does, and I actually use space more often because it's usually easier to reach than =. So, I'm not sure this is that good of a change.

Can we say somewhere that space is admissible in the long form? Also, in -b DEPENDENCIES or -bDEPENDENCIES I'd leave just one and say somewhere that the other is admissible.

Copy link
Collaborator Author

@philderbeast philderbeast Jul 10, 2023

Choose a reason for hiding this comment

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

The --build-depends does work without the = (with the ) but the --jobs of cabal build all --jobs 1 doesn't so to be on the safe side I used = for long options (and for consistency to match the --help output). Are there any long options that don't work with = (in the "space" where some people might use )?


Set verbosity level (0–3, default is 1).
Control verbosity (n is 0--3, default verbosity level is 1).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does our doc engine support things like --? I'm afraid that you need to manually insert the .

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps the 0--3 in the console is the mistake (an artifact)? I've never seen that before verbatim in plain text when 0..3 seems the more usual (or even 0-3).

I see that -- is converted to en-dash by smart quotes of sphinx.

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Terrific! I left a bunch of comments but they're not critical. We'll need to search for a second reviewer...

* WARNING: unknown option: '--prog-option'
* WARNING: unknown option: '--prog-options'
@ulysses4ever
Copy link
Collaborator

ulysses4ever commented Jul 12, 2023

Doing a massive review request from @andreabedini @Mikolaj @Kleidukos @ffaf1 @geekosaur because this has been lingering for a while.

@ulysses4ever ulysses4ever added the squash+merge me Tell Mergify Bot to squash-merge label Jul 12, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jul 14, 2023
@ulysses4ever
Copy link
Collaborator

Merge delay has passed. Thanks everyone who took a look at it and OKed it, and of course, to the author!

Since GitHub doesn't support updating other people's branches in forks belonging to organizational accounts, I'll have to merge it manually (#9108 will improve this workflow a bit, hopefully).

Backport. This patch is mostly about documentation, but it touches the code related to cabal flags. Therefore, I think it doesn't qualify for a backport. Just to be on the safe side.

@ulysses4ever ulysses4ever merged commit ca5d19f into haskell:master Jul 14, 2023
42 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants