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

Use links to appendices' commands. #3037

Closed
wants to merge 1 commit into from
Closed

Conversation

kinow
Copy link
Member

@kinow kinow commented Mar 23, 2019

A few days ago there was a discussion about dropping the cylc $COMMAND documentation, where we decided to keep it.

Today while I was reading our documentation for external triggers, I noticed a part that says

The function arguments mirror the arguments and options of the cylc suite-state command - see cylc suite-state --help for documentation.

And thought since we have documentation for what cylc suite-state --help prints, we could simply link to it? I tried a very simple :ref:command-suite-state`` but that did not work, giving me the error:

WARNING: undefined label: command-suite-state (if the link has no caption the label must precede a section header)

Being quite novice at Sphinx-Fu, I tried to do exactly as Sphinx's error message suggested, and moved the link before the section header in make-commands.sh. Not sure if the other way was intentional, or if I am breaking anything.

Tested locally generating with cylc make-docs.

@kinow kinow added the doc Documentation label Mar 23, 2019
@kinow kinow added this to the cylc-8.0a1 milestone Mar 23, 2019
@kinow kinow self-assigned this Mar 23, 2019
@kinow
Copy link
Member Author

kinow commented Mar 23, 2019

One problem with this approach is that the rendered link text displays the section text, which normally is not the same as cylc run --help, but instead simply run.

@hjoliver
Copy link
Member

I'm afraid I have no Sphinx-Fu yet; maybe @oliver-sanders or @sadielbartholomew can comment.

@oliver-sanders
Copy link
Member

Sphinx-Fu!

Sphinx encourages you to document objects in a structured manner.

tldr

Ideally we should use the standard Sphinx approach using the program, option and envvar directives.

Sphinx Domains

When we document a Python function, rather than doing it in plain text like this:

Take a look at :ref:`my_function`.

.. _my_function:

``my_function(arg1)``
---------------------

Description of function.

arg1
    Description of arg1.

We instead use the Python "domain" which is prefixed py:, like this:

Take a look at :py:function:`my_function`.

.. py:function:: my_function(arg1)

   Description of function.

   :param arg1: Description of arg1.

Sphinx domains provide metadata about the thing we are documenting, this can be used by the template for presentation but these objects can also be shared between different Sphinx projects. For example we could do the following in the Rose documentation:

Take a look at the :py:class:`cylc.scheduler.Scheduler` class.

The Standard Domain

For command line programs Sphinx provides the standard domain e.g:

On the command line use the :program:`cylc` executable.

.. program:: cylc

   A CLI utility for working with, inspecting and running Cylc suites.

There are Sphinx extensions which can automatically do this for CLI's implemented in argparse e.g. auto program. Unfortunately it looks like we might be moving away from argparse but we could probably write a quick extension to do this in whatever framework we move to, it's not that hard, we've already written an auto-documenter for Rose.

How Rose Does It

Rose does it the wrong way (because we were stuck with optparse when we wrote it)! When we change argument parser we should move to a nicer system.

https://github.com/metomi/rose/blob/2019.01.0/sphinx/ext/auto_cli_doc.py

@kinow
Copy link
Member Author

kinow commented Mar 25, 2019

🤓 I'll have to consult the sphinx manual a do a bit of training later. Really feeling like a young grasshopper here.

Ideally we should use the standard Sphinx approach using the program, option and envvar directives.

Hmmm, do you think we can improve the current approach, or would it be a broader change for later?

Unfortunately it looks like we might be moving away from argparse but we could probably write a quick extension to do this in whatever framework we move to

Anything but optparse is fine for me. I remember reading a comment some time go about limitations in argparse, but if integration with Sphinx is an advantage, and if we can use the cli_function you created with the same features as in other tools, that may become a good alternative too.

Thanks for the detailed explanation @oliver-sanders! I will re-read it later this week, especially the part about domains.

@oliver-sanders
Copy link
Member

Hmmm, do you think we can improve the current approach, or would it be a broader change for later?

The big bit of work here is going through the Cylc user guide and making changes like this:

- Then run the ``cylc scan`` command.
+ Then run the :program:`cylc scan` command.

This shouldn't change no matter how we generate the CLI stuff, whether via an argparse plugin or a custom script (as we are currently doing). So I guess this is something we could do now and improve later. It might make life easier to do this after #2972 though.

@oliver-sanders
Copy link
Member

Shall we punt this to an issue with #2972 as a dependency?

@hjoliver hjoliver mentioned this pull request Apr 7, 2019
@hjoliver
Copy link
Member

hjoliver commented Apr 7, 2019

OK, from a quick re-read above, @kinow 's fix here doesn't render the link as we want, and it's a bigger job to do it properly ... so I'll close this and create an issue dependent on #2972 as @oliver-sanders suggests.

@hjoliver hjoliver closed this Apr 7, 2019
@matthewrmshin matthewrmshin removed this from the cylc-8.0a1 milestone Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation superseded
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants