suite.rc syntax lexers: improve emacs support

[sadielbartholomew]

The emacs editor is lacking the basic level of syntax highlighting all-inclusive support provided for the other editors supported to some extent (see first 7 rows of the table given in #2752). In particular, Jinja2 {% ... %} & multi-line statements should at least be fully supported & this is implemented here.

Equality for emacs:heavy_exclamation_mark:

[hjoliver]

I'm all for fair treatment of the downtrodden and the disadvantaged. 🙌 so I'm approving this in principle.

However, I don't use emacs - so I'm going to have to delegate the proper review to someone who does!

[sadielbartholomew]

Neither does @oliver-sanders, but I would think one would simply need to check the syntax highlighting looks correct, which simply requires opening, eyeballing & closing the file, which is trivial unlike for vim! But if you would like to feel free to delegate.

[hjoliver]

Ha 😁 fair enough. Personally I think typing "[esc]:q" is vastly easier than "[ctrl-X][ctr-C]" (see I do remember some emacs, from back when it was better than vim ... 25 years ago! I'll try to take a look...

[hjoliver]

I'm not sure this works as advertised? For a small suite, here's current master:

and on this branch:

The only difference appears to be that Jinja2 print statements {{I}} are no longer highlighted inside the graph string??

[hjoliver]

@sadielbartholomew - what do you mean by "multi-line statements"? Multi-line strings or continuation-lines, in suite.rc syntax? or multi-line Jinja2 statements? ... Also note that Jinja2 {% ... %} seems to be highlighted already on current master, according to my expt above.

[sadielbartholomew]

Sorry if this did not work with your set-up @hjoliver! I am trying to think why we could be getting different results, as with mine & Oliver's set-up there were definitely issues with the emacs syntax highlighting & my code definitely fixed them to some extent (see detail below). Any ideas why we get different outputs, as it shouldn't depend on the Linux environment?

The issues we see:

• the original line 17 of cylc-mode.el has no effect, so that any {% .. %} highlighting (see below) is a result of line 46.
• {% .. %} is highlighted only in certain cases, which I diagnosed to be whenever there are only alphanumeric characters inside the statement. If there are punctuation characters such as = (commonly for variable instantiation), or quotation marks, statements are not recognised. Hence why I added [[:punct:]] into the regex which resolved this in my setup.
• Multi-line Jinja2 statements (sorry for the ambiguity, this is what I meant by multi-line statements), e.g:

  {% set stations = {'belmullet': 3976,
                     'camborne': 3808,
                     'heathrow': 3772,
                     'shetland': 3005} %}
  

  are not recognised even when single-line punctuation-inclusive statements are. I believe that {%-enclosed statements with { for line continuations are the only possible Jinja2 multi-line(?), though it is hard to find the info on this in their docs. I added in a variation on \n.*? to deal with this that also worked for my set-up.

Up-to-date master branch:

This (PR) branch:

However as can be seen the {{ ... }} expressions do also lose their highlighting with my current change, so I will squash in a fix for this shortly. Sorry for missing that.

[sadielbartholomew]

Update: the {{ ... }} expressions had simply broken because in the final step of my first implementation I attempted to break up what it currently line 46 which is a regular expression over ~80 chars max length. Line continuation is not trivial in Emacs lisp, & as I have just taken around 1.5 hours to try to successfully implement the defining & concatenating of sub-strings (I have given up). With the line unbroken, which is not ideal style-wise but not crucial, this works, & have updated the PR as such.

I think it best if we both show what we get on master & on this branch with default emacs styling etc., & determine & share our versions, so any differences are explicit. I'll do that tomorrow morning. If you could too when you get a chance that would be appreciated, thanks.

[sadielbartholomew]

Okay, so I am using the same (hacked-together & with imperfect suite design, but capturing everything wanted for testing) example as above. Could you please copy & paste it to show what you get for the same code, as below,

        [[[ T00,T02,T04 ]]]
            # a comment
            graph = """
{% if TEST_SUITE == 'developer' %} 

	                swe_start => swe_calcwsa
                              => swe_getwsaout
                              => swe_wsadu2bc
                              => swe_cone2bc
                              => swe_enlil
                              => swe_postpro

{% elif TEST_SUITE == 'developer_ens' %}

{% for ma in MEMBERS_ADAPT %}

{% set ma = '%02d' % ma %}
		            swe_start_R{{ma}} => swe_calcwsa_R{{ma}} 
                                      => swe_getwsaout_R{{ma}} 
                                      => swe_wsadu2bc_R{{ma}} 
                                      => swe_cone2bc_R{{ma}} 
                                      => swe_enlil_R{{ma}} 
                                      => swe_postpro_R{{ma}}

                """
{% endfor %}
{% endif %}

{% set nums = {'swe_calcwsa_R': swe_calcwsa,
               'swe_getwsaout_R': swe_getwsaout,
               'swe_wsadu2bc_R': swe_wsadu2bc,
               'swe_cone2bc_R': swe_cone2bc} %}

with nothing in your .emacs except for:

;; Cylc syntax
(add-to-list 'load-path "~/.emacs.d/lisp/")
(require 'cylc-mode)
(setq auto-mode-alist (append auto-mode-alist 
    (list '("\\.rc$" . cylc-mode))))

With that example & that init file, I now (after fixing the {{ ... }} issue as commented above) get, using version GNU Emacs 23.1.1, determined from the command emacs --version):

Up-to-date master branch:

This (PR) branch:

[hjoliver]

@sadielbartholomew - thanks for the extra work on this. I won't bother posting screenshots because my results now agree with yours. I therefore re-affirm my approval of this PR 👍

[oliver-sanders]

Nearly there, just a couple niggles:

Multiline blocks ({%) now work great with punctuation, can we extend this to comments ({#):

{# foo
   "bar"
   baz #}

Pipe characters through a spanner into the works:

{{ x|upper() }}

[sadielbartholomew]

@oliver-sanders ah I wasn't aware of multi-line comments, thanks for raising that, & well-spotted RE the pipe. I'll get this amended accordingly.

[sadielbartholomew]

I've been working on this, but I am not quite finished & will comment when ready for final re-review. It seems there are further inconsistencies with gvim, which we can take as the standard for Cylc syntax highlighting.

New more comprehensive test:

# --------------------Quoting and equals signs:
# wow = wow
'wow' = "wow"
wow == wow
# -------------General, with Jinja2 statements:
[[ something, something else ]]
        [[[ T00,T02,T04 ]]]
            graph = """
{% for ma in MEMBERS_ADAPT %}
{% if TEST_SUITE == 'developer' %}
	                swe_start => swe_calcwsa
                              => swe_getwsaout
{% set ma = '%02d' % ma %}
		            swe_start_R{{ma}} => swe_calcwsa_R{{ma}}
                                      => swe_getwsaout_R{{ma}}
{% set nums = {'swe_calcwsa_R': swe_calcwsa,
               'swe_getwsaout_R': swe_getwsaout,
               'swe_cone2bc_R': swe_cone2bc} %}
                """
{% set nums = {'swe_calcwsa_R': swe_calcwsa,
               'swe_getwsaout_R': swe_getwsaout,
               'swe_cone2bc_R': swe_cone2bc} %}
{% endfor %}
{% endif %}
{{ x|upper() }}
# -----------------------------Jinja2 comments:
{# new attempt #}
{# new attempt
   here it is
   'wow' #}
{# new attempt
   here it is
   wow #}
{# new attempt
   here it is
   "wow" #}

gvim standard:

master:

current state of PR (only last 3 lines need fixing):

Just those last 3 lines to fix.

[sadielbartholomew]

After discussions with Oliver, we think it is best to break consistency with vim for something that seems more intuitive, namely for Jinja2 comments to appear in the 'comment face' as for regular comments, not as standard Jinja2 face. Other than that this should be consistent with the vim screenshot above.

For the same example text, after the latest commit I get:

I still have no idea why there is a lone Jinja2 comment that does not seem to play ball wherever I place it in the text & whether or not its contents includes quotes! I have spent hours trying to uncover the reason (via tweaking all the regexes, using an online debugger, etc.) for this to no avail. Anyone have any ideas?

Otherwise, I have spotted that sections with punctuation are also broken on master for emacs. I still need to fix that (too), & it isn't trivial as there are inter-cycle offsets to distinguish from in square brackets. Will return to this Monday.

[hjoliver]

Sounds difficult! I wonder if it is worth asking emacs experts (on some support mailing list) about this (font-lock mode and the "lone Jinja2 comment..." bit above)?

[hjoliver]

@cylc/core - if the final problem above proves too hard to solve, I think we should consider merging this as-is - it is still a significant improvement on current emacs support - in order to get the release out. It can then be tweaked in a later PR.

[sadielbartholomew]

@hjoliver: sorry I was focusing on other work previously, but I know this is now urgent to finalise to some extent for the next release.

I'm consulting an emacs 'expert' (not sure whether quoting this is appropriate 😄) who may be able to detect the remaining issue, as described above. If we can't debug it today I agree we should merge as-is (with any simple possible further feedback addressed).

[sadielbartholomew]

@stevewardle if you have some spare time for this today, could you please see if you can see any reason for the following:

there is a lone Jinja2 comment that does not seem to play ball wherever I place it in the text & whether or not its contents includes quotes! I have spent hours trying to uncover the reason (via tweaking all the regexes, using an online debugger, etc.) for this to no avail.

There should be sufficient info in the PR comments & code to comprehend the problem, but I am happy to summarise it for you in-person, or show the strange behaviour as it appears on my monitor, if that would make it easier? Thank in advance.

[stevewardle]

Hmm... this all feels fairly familiar as I recall running into a lot of similar oddities when writing the syntax file for Rose... but there are a few other things about this Cylc mode that I think are worth fixing too:

• It's kind of strange that some of the faces are set within the mode definition and then an extra one is stuffed on afterwards via a hook...
• I don't think setting "font-lock-defaults" is right, this actually pollutes the syntax highlighting outside of the mode! (you can see this if for example you open a file which uses Cylc mode and then open a different file like a Fortran file or an elisp file the highlighting is messed up)
• Some of the regex's seem like they might be redundant (though I'm not sure precisely what the desired result is) easiest way to check these is within emacs itself using the "re-builder" command

As for a fix I think perhaps it might be worth copying the Rose syntax file (but strip out the mode-specific parts and replace them with the regex's from this change)? At a glance there are a few bits in the Rose one which might fix the issue above (I recall having a similar problem trying to get the trigger-ignored sections to turn grey)

[sadielbartholomew]

Thanks for having a look @stevewardle, that is very helpful.

Admittedly I based my solution on the syntax highlighting file as it was before, so assumed the use of e.g. font-lock-defaults and the final hook were legitimate strategies. Also I am fairly new to regular expressions, so am not surprised if the ones I added have issues (I wasn't aware of the re-builder command, that should be really useful).

Using your Rose syntax file as a template sounds like the way to, I agree. I'll give that a shot this afternoon.

[sadielbartholomew]

Update

I can't thank you enough @stevewardle (:star:); after I copied all the regular expressions I had adapted so far into the Rose syntax template as you suggested and amended it for Cylc / suite.rc files accordingly, all the strange highlighting glitches vanished!

From there it was really easy to simplify the regular expressions I already had. I think the reason they had become so convoluted was because I was trying to manipulate the oddness incorporated somewhere within the original approach. Had I begun with that template I feel I could have done this is a tenth of the time 😑. But it was a learning process, that's for sure.

I can't seem to add you as a reviewer, as you aren't in the cylc/core group (?), but feel free to informally review this.

All: this is open for (hopefully final) reviewing.

Reviewing aides

It's perhaps overkill, but I'll provide up-to-date screenshots since there are a few more domain cases I didn't cover with my previous mock (bad but illustrative) suite.rc file, namely punctuation within section headers and inter-cycle dependencies. I added in [scheduling], [[[+PT6H/PT6H]]] & [-PT1H30M] and [^] to lines 6, 8, 11 & 14 of the example from my copyable comment on October 12th.

After the latest commit, I now (with the minimal .emacs, as also outlined & copyable from that PR comment) get on this branch:

re-reviewing...

[hjoliver]

Re-approving, after the new implementation (disclaimer, again: I haven't been an emacs user for 20 years).

[stevewardle]

Also looks good to me - glad to see you got it all working in the end 😌

[oliver-sanders]

Looks good and has some big improvements, I especially like the highlighting of => symbols which makes line break type issues more obvious to spot.

I've found a few new niggles, some of which should be quick to fix:

• Settings with hyphens break highlighting (e.g. clock-trigger)
• In-line comments are now broken (e.g. foo=Foo # in-line comment, Rose doesn't support these which is probably why)
• Multi-line strings no longer get special highlighting. The vi syntax file doesn't highlight multi-line strings specially but if the quotes are missing or wrong it makes it clear (e.g. graph = """\nfoo\n[runtime]...)
• Square brackets cause issues (e.g. {% set foo = [] %})
• Jinja2 doesn't get highlighted inside sections (e.g. [[foo{{ foo }}]])

[sadielbartholomew]

Very thorough reviewing @oliver-sanders! I wasn't even aware Jinja2 could be used inside sections. I'll get those fixed now.

Rose doesn't support these which is probably why

No, I used the template from the Rose syntax but created the regular expressions from scratch myself. So the actual highlighting behaviour is largely custom, & hopefully I can adapt it easily.

I will check with you about multi-line string 'quoting' behaviour in person as I am not sure what is best to implement there.

[sadielbartholomew]

@oliver-sanders I have now as far as I can tell covered the issues you spotted. For testing I used:

# --------------------Quoting and equals signs:
# wow = wow
'wow' = "wow"
wow == wow  # in-line comment
# -------------General, with Jinja2 statements:
[scheduling]
[[ something, something_how else ]]
        [[[ T00,T02,T04 ]]]
[{% foo %}foo]
[ fo o{# foo #}foo]
[foo-how,_wow{{ foo }}foo]  # in-line comment
    [[{% foo %}foo]]
    [[ fo o{# foo #}foo]]
    [[foo-how,_wow{{ foo }}foo]]
        [[[{% foo %}foo]]]
        [[[ fo o{# foo #}foo]]]
        [[[foo-how,_wow{{ foo }}foo]]]
            graph = """
{% for ma in MEMBERS_ADAPT %}
	                swe_start[-PT1] => swe_calcwsa  # in-line comment
                              => swe_getwsaout
{% set ma = '%02d' % ma %}
		            swe_start_R[^]{{ma}} => swe_calcwsa_R{{ma}}
                                      => swe_getwsaout_R{{ma}}
               """
        clock-trigger = swe_start_R(PT1H)
{% set nums = {'swe_calcwsa_R': swe_calcwsa,  # in-line comment
               'swe_getwsaout_R': swe_getwsaout,
               'swe_cone2bc_R': swe_cone2bc} %}
clock-trigger = """ hello = why
                """
this = that
that = """
this
"""

For multiline strings, I went for the simple solution of highlighting any occurence of the pattern """. Eventually, we could put in something fancier to break any succeeding highlighting when triple quotes are not closed, but I left this out here as I am finding it not trivial to accomplish.

[oliver-sanders]

👍