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

Clean up built-in xtrigger docstrings #5840

Closed
wants to merge 1 commit into from

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Nov 22, 2023

Accompanies but does not depend on cylc/cylc-doc#673

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • No dependency changes
  • No tests needed
  • No changelog needed
  • Docs
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@MetRonnie MetRonnie added this to the cylc-8.2.4 milestone Nov 22, 2023
@MetRonnie MetRonnie self-assigned this Nov 22, 2023
>>> xrandom(0, 0)
(False, {})

.. code-block:: python
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't have to do this.

The Napoleon extension should automatically interpret the examples section as a Python code-block.

Copy link
Member

@oliver-sanders oliver-sanders Nov 23, 2023

Choose a reason for hiding this comment

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

But we aren't using Napoleon in cylc-doc, only Rose.

Suggest adding this into the extension list in cylc-doc:

'sphinx.ext.napoleon'

It's a built-in plugin, but it's not enabled by default, it understands the stricture of Google style docstring (what we use) and formats them more nicely, e.g:

https://metomi.github.io/rose/doc/html/api/rose-macro.html#metomi.rose.macro.MacroBase.get_metadata_for_config_id

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately it seems to be choking on non-built-in exceptions in "Raises" sections of doctrings.

cylc/flow/network/client.py:docstring of cylc.flow.network.client.WorkflowRuntimeClient:1: WARNING: py:exc reference target not found: WorkflowStopped
cylc/flow/network/client.py:docstring of cylc.flow.network.client.WorkflowRuntimeClientBase.serial_request:1: WARNING: py:exc reference target not found: ClientTimeout
cylc/flow/network/client.py:docstring of cylc.flow.network.client.WorkflowRuntimeClientBase.serial_request:1: WARNING: py:exc reference target not found: ClientError
cylc/flow/jinja/filters/strftime.py:docstring of cylc.flow.jinja.filters.strftime.strftime:1: WARNING: py:exc reference target not found: ISO8601SyntaxError
cylc/flow/jinja/filters/strftime.py:docstring of cylc.flow.jinja.filters.strftime.strftime:1: WARNING: py:exc reference target not found: StrftimeSyntaxError
cylc/flow/jinja/filters/duration_as.py:docstring of cylc.flow.jinja.filters.duration_as.duration_as:1: WARNING: py:exc reference target not found: ISO8601SyntaxError

Doing this doesn't work:

 Raises:
-    WorkflowStopped: ...
+    cylc.flow.exceptions.WorkflowStopped: ...

Copy link
Member

Choose a reason for hiding this comment

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

I think py:exc reference target not found means that it is trying to cross-reference the exception, but can't because the exception hasn't been documented.

Normally this wouldn't matter, but we turn on nit-picky mode so that referencing errors cause failures.

One option would be to auto-document exceptions in the reference section. I guess this could have some value.

Copy link
Member

Choose a reason for hiding this comment

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

[unrelated] Skimming the Rose config I spotted we're using sphinx.ext.viewcode there which is kinda cool.

@oliver-sanders oliver-sanders modified the milestones: cylc-8.2.4, 8.2.5 Jan 8, 2024
@oliver-sanders oliver-sanders marked this pull request as draft January 22, 2024 10:38
@oliver-sanders oliver-sanders mentioned this pull request Jan 26, 2024
8 tasks
@MetRonnie
Copy link
Member Author

Superseded by #5955

@MetRonnie MetRonnie closed this Feb 20, 2024
@MetRonnie MetRonnie deleted the xtrigger-doc branch February 20, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants