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

dependency graph: deprecate the continuation backslash? #1328

Closed
matthewrmshin opened this issue Feb 12, 2015 · 7 comments
Closed

dependency graph: deprecate the continuation backslash? #1328

matthewrmshin opened this issue Feb 12, 2015 · 7 comments

Comments

@matthewrmshin
Copy link
Contributor

Background:

  • Trailing spaces after the backslash can result in some puzzling errors for users.
  • The requirement of the backslash in dependency graphs for continuation can result in some rather interesting looking logic when combined with usage of Jinja2. E.g.:
breakfast => lunch{% if is_still_hungry %} \{% endif %}
{% if is_still_hungry %}
=> high-tea => snack => dinner => snack => supper
{% endif %}

Is the backslash actually necessary? Continuation should be obvious from the graph syntax. If an identifier is followed by a =>, & or | operator, then it is still in the same statement. If an identifier is followed by another identifier, the second identifier is the beginning of a new statement. E.g.: it should be obvious that the following are equivalent:

t1 => t2  t3 => t4
t1 => t2
t3 => t4
t1 =>
t2
t3 => t4
t1
=> t2  t3 =>t4

The Jinja2 example can be simplified somewhat:

breakfast => lunch
{% if is_still_hungry %}
=> high-tea => snack => dinner => snack => supper
{% endif %}

Comments?

@matthewrmshin matthewrmshin self-assigned this Feb 12, 2015
@matthewrmshin matthewrmshin added this to the later milestone Feb 12, 2015
@hjoliver
Copy link
Member

@matthewrmshin - I agree that line continuation markers are not really necessary in graph strings, but the line-continuation is done early in the parsing process (in lib/parsec/fileparse.py) when the file is still being handled simply as a list of arbitrary text lines - so the parser does not know if it's inside a graph string or not. Also, there's some potential for confusion in the other direction, by allowing line continuation anywhere else in the file, but not inside graph strings.

I have limited sympathy for users who leave whitespace after a line continuation marker - that sort of behaviour will bite them in their scripting etc. too.

By the way, in the graph we don't strictly need line continuation or your still-inside-a-statement check (although I agree that would be nice) as already you can just do this:

breakfast => lunch
{% if is_still_hungry %}
lunch => high-tea => snack => dinner => snack => supper
{% endif %}

@matthewrmshin
Copy link
Contributor Author

I think parsec actually has some special logic for handling dependency graphs because the continuation marker is removed by the time the graph is processed by cylc.config. It does not remove the continuation marker for something like a shell script.

I don't have a lot of sympathy for the trailing white spaces problem, but then again we are dealing with real users here, so anything to help them is good.

A better Jinja2 example would be one where the first line is full of | logic, with some pretty long identifiers, making it difficult to not do a continuation.

@matthewrmshin
Copy link
Contributor Author

I think parsec actually has some special logic for handling dependency graphs because the continuation marker is removed by the time the graph is processed by cylc.config. It does not remove the continuation marker for something like a shell script.

I take it back. It looks like the continuation marker is removed from command scripting as well.

However, it appears that we already have some special logic to merge graphs under parsec.fileparse.

@hjoliver
Copy link
Member

real users here, so anything to help them is good.

Sure, if the cost/benefit analysis works out in favour :-) In this case it might be easy enough, but I'm just pointing out that currently line joining is done globally, before we know anything about the meaning of the lines involved.

However, it appears that we already have some special logic to merge graphs under parsec.fileparse

Yes - see #542. This was one of the reasons for replacing ConfigObj.

@benfitzpatrick
Copy link
Contributor

I am in favour of updating the graph parsing engine, and not requiring the backslash.

I don't think it is worth deprecating (then removing support for) the backslash on any
reasonable timescale. Cylc 6 syntax is taking ages to migrate to, even when it has clear
benefits and justification. What is the clear benefit to the user when they replace their
backslashes?

@hjoliver
Copy link
Member

I'm not sure the cost/benefit works out because:

  • continuation lines are easier to handle as now as a sort of global preprocessing step - the guts of the parser never sees them, just the joined lines.
  • they are already not required in the graph to split up long dependency chains - as per my version of Matt's example above
  • if we still want to allow them in other parts of the file (which does not seem unreasonable?) then disallowing them inside graph strings creates an inconsistency that could be confusing.

I do think we should support syntax-driven line continuation in the graph, as you've suggested, which would make backslashes doubly unnecessary (in the graph string) but doesn't preclude allowing them as well for the three reasons just above.

Ah... I just re-read Ben's comment and I think he's saying the same thing as me (?at least on "any reasonable timescale" he is).

@matthewrmshin matthewrmshin removed their assignment Jun 16, 2016
@hjoliver hjoliver removed this from the later milestone Jun 21, 2016
@hjoliver
Copy link
Member

[meeting]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants