-
Notifications
You must be signed in to change notification settings - Fork 19
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
Disambiguate 'active tasks'. #798
base: master
Are you sure you want to change the base?
Conversation
982d057
to
56a9107
Compare
56a9107
to
7af814f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I seem to have gone a bit bonkers on the written English side of the review. Suggestions made after reading text out loud.
Please take or leave my suggestions.
Is it worth checking cylc-flow/flow/cylc/scripts
to ensure those descriptions are updated in line with this?
|
||
- waiting tasks are not pre-spawned before they are needed | ||
- succeeded tasks are not kept across the active task window | ||
- succeeded tasks are not retained across the :term:`active window` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
niff-naff style point - are these bullets sentences?
- succeeded tasks are not retained across the :term:`active window` | |
- tasks the :term:`active window` when they succeed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can never quite decide if short bullet points should be styled as full sentences or not. I'll rephrase along the lines of your suggestion (can't take it as-is as it seems to be malformed!).
src/glossary.rst
Outdated
The GUI provides a view extending ``n`` (default ``n=1``) | ||
graph edges out from the :term:`active window` of the workflow. | ||
Thus in the context of the GUI the active window may also be | ||
referred to as the `n=0` window. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GUI provides a view extending ``n`` (default ``n=1``) | |
graph edges out from the :term:`active window` of the workflow. | |
Thus in the context of the GUI the active window may also be | |
referred to as the `n=0` window. | |
By default the GUI provides a view extending ``n`` (default ``n=1``) | |
graph edges out from the :term:`active window` of the workflow. | |
In the context of the GUI the active window may also be | |
referred to as the `n=0` window. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll skip this suggestion, because:
- "By default the GUI ... extending n (default n=1) edges": too much "default"!
- The first sentence provides the reason for the second, hence they should be connected by "Thus" or "So".
It is now possible to change the window extent in the GUI via a button in the | ||
toolbar allowing you to see tasks further back in the workflow's history. | ||
You can change the n-window extent in the GUI with a toolbar button, to display | ||
fewer or more tasks around the current ``n=0`` :term:`active window`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you should have corrected me if I'd used "less" there! Good writing is about more than just conveying meaning.
Tasks in the :term:`active window` already have flow membership assigned | ||
(namely that of the parent tasks that spawned them). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tasks in the :term:`active window` already have flow membership assigned | |
(namely that of the parent tasks that spawned them). | |
Tasks in the :term:`active window` already have flow membership | |
assigned. Flow membership is inherited from their parent tasks | |
when they are spawned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repeated words and phrases, in close proximity, when the subject is clear from the context kind of stand out as bad style, to me, which is distracting when reading. In this case, however, I think I'll take your suggestion with a slight tweak (I don't like the "their" there), since the second sentence sounds like an important statement in its own right.
Co-authored-by: Tim Pillinger <[email protected]>
Thanks @wxtim - I took most of you suggestions, or variants thereof, and argued against several. |
750ec3d
to
3ff2cc7
Compare
Successful build requires cylc/cylc-flow#6618 |
We're still overloading the term "active tasks" to mean both:
This PR ensures that "tasks in the active window" is used in the first case, and clarifies the definition for both cases.
(Note we do still need both concepts - some things depend on "task activity" in the more literal sense of submitted jobs; some things depend on the wider active window - but we should not use the same term for both, because they are different).
Successful build requires cylc/cylc-flow#6618
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.