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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cylc/flow/xtriggers/wall_clock.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@
from time import time


def wall_clock(trigger_time=None):
def wall_clock(trigger_time: int) -> bool:
"""Return True after the desired wall clock time, False.

Args:
trigger_time (int):
trigger_time:
Trigger time as seconds since Unix epoch.
"""
return time() > trigger_time
4 changes: 3 additions & 1 deletion cylc/flow/xtriggers/workflow_state.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ def workflow_state(
message: Optional[str] = None,
cylc_run_dir: Optional[str] = None
) -> Tuple[bool, Optional[Dict[str, Optional[str]]]]:
# NOTE: docstring must be valid rst as this is included in the docs
"""Connect to a workflow DB and query the requested task state.

* Reports satisfied only if the remote workflow state has been achieved.
Expand All @@ -55,6 +56,7 @@ def workflow_state(
The task status required for this xtrigger to be satisfied.
message:
The custom task output required for this xtrigger to be satisfied.

.. note::

This cannot be specified in conjunction with ``status``.
Expand All @@ -72,7 +74,7 @@ def workflow_state(
tuple: (satisfied, results)

satisfied:
True if ``satisfied`` else ``False``.
``True`` if ``satisfied`` else ``False``.
results:
Dictionary containing the args / kwargs which were provided
to this xtrigger.
Expand Down
40 changes: 25 additions & 15 deletions cylc/flow/xtriggers/xrandom.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@


def xrandom(percent, secs=0, _=None):
# NOTE: docstring must be valid rst as this is included in the docs
"""Random xtrigger, with configurable sleep and percent success.

Sleep for ``sec`` seconds, and report satisfied with ~``percent``
Sleep for ``sec`` seconds, and report satisfied with ~ ``percent``
likelihood.

The ``_`` argument is not used in the function code, but can be used to
Expand All @@ -43,28 +44,37 @@ def xrandom(percent, secs=0, _=None):
Examples:
If the percent is zero, it returns that the trigger condition was
not satisfied, and an empty dictionary.
>>> 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.


>>> xrandom(0, 0)
(False, {})

If the percent is not zero, but the random percent success is not met,
then it also returns that the trigger condition was not satisfied,
and an empty dictionary.
>>> import sys
>>> mocked_random = lambda: 0.3
>>> sys.modules[__name__].random = mocked_random
>>> xrandom(15.5, 0)
(False, {})

.. code-block:: python

>>> import sys
>>> mocked_random = lambda: 0.3
>>> sys.modules[__name__].random = mocked_random
>>> xrandom(15.5, 0)
(False, {})

Finally, if the percent is not zero, and the random percent success is
met, then it returns that the trigger condition was satisfied, and a
dictionary containing random color and size as result.
>>> import sys
>>> mocked_random = lambda: 0.9
>>> sys.modules[__name__].random = mocked_random
>>> mocked_randint = lambda x, y: 1
>>> sys.modules[__name__].randint = mocked_randint
>>> xrandom(99.99, 0)
(True, {'COLOR': 'orange', 'SIZE': 'small'})

.. code-block:: python

>>> import sys
>>> mocked_random = lambda: 0.9
>>> sys.modules[__name__].random = mocked_random
>>> mocked_randint = lambda x, y: 1
>>> sys.modules[__name__].randint = mocked_randint
>>> xrandom(99.99, 0)
(True, {'COLOR': 'orange', 'SIZE': 'small'})

Returns:
tuple: (satisfied, results)
Expand Down
Loading