graph line continuation on boolean #4404

wxtim · 2021-09-10T11:34:59Z

These changes close #4390

Requirements 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).
Appropriate tests are included (unit and/or functional).
Additionally refactored unittest test to pytest.
Appropriate change log entry included.
(master branch) I have opened a documentation PR at document graph line continuation on boolean cylc-doc#296.

I have left commits separate to make changes to the test easier to understand. If merging please squash.

cylc/flow/graph_parser.py

hjoliver

(The one test that's failing should be an easy fix: tests/f/validate/62-null-task-name.t)

cylc/flow/graph_parser.py

hjoliver · 2021-09-16T03:58:35Z

(Oops, didn't notice this was still a draft PR, so maybe you handn't finished 😬 )

wxtim · 2021-09-17T18:24:24Z

Only because that test broken and I'm holiday

Removed broken functional test of trailing &, which is now legal. Added unit test for error if last line ends with a permitted continuation.

wxtim · 2021-09-21T09:04:16Z

@hjoliver can you pay particular attention to whether my merge with your recent master changes is OK?

tests/unit/test_graph_parser.py

hjoliver

Still not getting consistent treatment of the three continuation items.

# OK:
 a =>  # GraphParseError: Trailing continuation sequence (=>):a=>

# OK, but use consistent terminology)
 => a  # GraphParseError: Leading arrow: =>a

# OK:
 a &  # GraphParseError: Trailing continuation sequence (&):a&
 a |  # GraphParseError: Trailing continuation sequence (|):a|

# Not OK:
 & a  # valid
 | a  # valid

# OK:
 => a => b  # GraphParseError: Leading arrow: =>a=>b

# Not OK:
 & a => b  # GraphParseError: Null task name in graph: &a => b
 | a => b  # WorkflowConfigError: ERROR: bad trigger

cylc/flow/graph_parser.py

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>

…n-on-and-or * upstream/master: Update cylc/flow/cfgspec/workflow.py clean up interim obselete message fix flake8-simplify failures caused by recently added checks. (cylc#4420)

wxtim · 2021-09-22T10:20:10Z

Still not getting consistent treatment of the three continuation items.

I there was 1 remaining case (leading continuation symbol) where the code was checking for => rather than the whole list. I think that I've fixed this now.

wxtim · 2021-09-22T10:21:23Z

tests/unit/test_graph_parser.py

@@ -49,17 +50,42 @@ def test_parse_graph_fails_null_task_name(graph):
        assert "Null task name in graph:" in str(cm.value)


+@pytest.mark.parametrize('seq', ('&', '|', '=>'))


Factored out of the original test to allow this double parametrization.

hjoliver

All working well now 👍 Just one code simplification suggestion.

cylc/flow/graph_parser.py

hjoliver · 2021-09-23T05:59:32Z

tests/unit/test_graph_parser.py

@@ -49,17 +50,42 @@ def test_parse_graph_fails_null_task_name(graph):
        assert "Null task name in graph:" in str(cm.value)


+@pytest.mark.parametrize('seq', ('&', '|', '=>'))


Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>

cylc/flow/graph_parser.py

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>

hjoliver

Thanks @wxtim all good 🎉

One review will do for this because: "time is of the essense" right now, plust it is somewhat peripheral functionality, and I've tested the hell out of it.

wxtim requested review from MetRonnie and kinow September 10, 2021 11:35

wxtim self-assigned this Sep 10, 2021

wxtim added could be better Not exactly a bug, but not ideal. small labels Sep 10, 2021

wxtim added this to the cylc-8.0.0 milestone Sep 10, 2021

wxtim added 3 commits September 10, 2021 12:39

refactored test to use pytest

6a8a533

add testing for line break on & and |

bad6687

fix graph_parser

3162307

wxtim force-pushed the feature.graph-join-on-and-or branch from c1524ad to 3162307 Compare September 10, 2021 11:41

CHANGES.md

2268b0d

wxtim mentioned this pull request Sep 10, 2021

document graph line continuation on boolean cylc/cylc-doc#296

Merged

1 task

wxtim marked this pull request as draft September 10, 2021 13:15

kinow reviewed Sep 10, 2021

View reviewed changes

cylc/flow/graph_parser.py Outdated Show resolved Hide resolved

hjoliver requested changes Sep 16, 2021

View reviewed changes

cylc/flow/graph_parser.py Outdated Show resolved Hide resolved

wxtim added 2 commits September 20, 2021 11:54

Fixed error where last line ends with any permitted continuation seq.

89aaf48

Removed broken functional test of trailing &, which is now legal. Added unit test for error if last line ends with a permitted continuation.

made consecutive lines start with continiation chars

04e04b9

wxtim requested review from kinow and hjoliver September 20, 2021 11:50

wxtim marked this pull request as ready for review September 20, 2021 15:15

wxtim force-pushed the feature.graph-join-on-and-or branch from 04e04b9 to 09533ee Compare September 21, 2021 08:43

merger with master

a36a1ae

wxtim force-pushed the feature.graph-join-on-and-or branch from 09533ee to a36a1ae Compare September 21, 2021 09:01

alphabetize imports

6a780ed

wxtim changed the title ~~Feature.graph join on and or~~ graph line continuation on boolean Sep 21, 2021

hjoliver reviewed Sep 22, 2021

View reviewed changes

tests/unit/test_graph_parser.py Outdated Show resolved Hide resolved

hjoliver requested changes Sep 22, 2021

View reviewed changes

cylc/flow/graph_parser.py Outdated Show resolved Hide resolved

wxtim and others added 4 commits September 22, 2021 07:49

Update cylc/flow/graph_parser.py

4989e1f

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>

Update tests/unit/test_graph_parser.py

cc7e584

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>

fixed check for leading & or |

8c88544

Merge remote-tracking branch 'upstream/master' into feature.graph-joi…

f9c61f7

…n-on-and-or * upstream/master: Update cylc/flow/cfgspec/workflow.py clean up interim obselete message fix flake8-simplify failures caused by recently added checks. (cylc#4420)

wxtim requested a review from hjoliver September 22, 2021 10:18

wxtim commented Sep 22, 2021

View reviewed changes

hjoliver requested changes Sep 23, 2021

View reviewed changes

wxtim and others added 2 commits September 23, 2021 07:16

Update cylc/flow/graph_parser.py

3e62709

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>

response to review suggestions from Hilary Oliver

b35af54

hjoliver reviewed Sep 23, 2021

View reviewed changes

cylc/flow/graph_parser.py Outdated Show resolved Hide resolved

hjoliver reviewed Sep 23, 2021

View reviewed changes

cylc/flow/graph_parser.py Show resolved Hide resolved

Update cylc/flow/graph_parser.py

defd344

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>

hjoliver approved these changes Sep 24, 2021

View reviewed changes

hjoliver merged commit 0d69823 into cylc:master Sep 24, 2021

hjoliver modified the milestones: cylc-8.0rc1, cylc-8.0b3 Sep 24, 2021

wxtim deleted the feature.graph-join-on-and-or branch September 24, 2021 06:02

This was referenced Oct 5, 2021

Selection of hosts and platforms cylc/cylc-doc#299

Closed

added a guide to writing a platform configuration. cylc/cylc-doc#289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

graph line continuation on boolean #4404

graph line continuation on boolean #4404

wxtim commented Sep 10, 2021 •

edited

Loading

hjoliver left a comment

hjoliver commented Sep 16, 2021

wxtim commented Sep 17, 2021

wxtim commented Sep 21, 2021

hjoliver left a comment •

edited

Loading

wxtim commented Sep 22, 2021

wxtim Sep 22, 2021

hjoliver Sep 23, 2021

hjoliver left a comment

hjoliver Sep 23, 2021

hjoliver left a comment

		@@ -49,17 +50,42 @@ def test_parse_graph_fails_null_task_name(graph):
		assert "Null task name in graph:" in str(cm.value)


		@pytest.mark.parametrize('seq', ('&', '\|', '=>'))

graph line continuation on boolean #4404

graph line continuation on boolean #4404

Conversation

wxtim commented Sep 10, 2021 • edited Loading

hjoliver left a comment

Choose a reason for hiding this comment

hjoliver commented Sep 16, 2021

wxtim commented Sep 17, 2021

wxtim commented Sep 21, 2021

hjoliver left a comment • edited Loading

Choose a reason for hiding this comment

wxtim commented Sep 22, 2021

wxtim Sep 22, 2021

Choose a reason for hiding this comment

hjoliver Sep 23, 2021

Choose a reason for hiding this comment

hjoliver left a comment

Choose a reason for hiding this comment

hjoliver Sep 23, 2021

Choose a reason for hiding this comment

hjoliver left a comment

Choose a reason for hiding this comment

wxtim commented Sep 10, 2021 •

edited

Loading

hjoliver left a comment •

edited

Loading