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

emacs syntax lexer: bad mix of greediness & laziness #2856

Merged
merged 1 commit into from
Nov 15, 2018

Conversation

sadielbartholomew
Copy link
Collaborator

@sadielbartholomew sadielbartholomew commented Nov 13, 2018

#2784 provides correct highlighting by eyeball, as tested, however I have since discovered through dev use that it is occasionally slowed, & sometimes hangs, in practice. I have traced this back to the typing of the opening but not yet closing marker for specific syntactical expressions, namely:

  • {# towards {# ... #}
  • {% towards {% ... %}

... & in turn to a mix of greedy & lazy quantifiers in the associated regular expressions, which have identical format bar the #/% distinction. This PR replaces those instances at fault.

(This bug discovery will be another lesson to label on my steep learning curve in regular expressions.)

Also amend one other issue manifest with regular use; a task with plain-text name on the first line of a graph string is recognised as a key for a key = value pair from not considering the subsequent >, e.g:

graph = """
    plaintexttask => not_just_plain_text_task
    """

highlights plaintexttask in the same colour as graph & not not_just_plain_text_task as makes sense contextually.

@sadielbartholomew sadielbartholomew added the bug Something is wrong :( label Nov 13, 2018
@sadielbartholomew sadielbartholomew added this to the next release milestone Nov 13, 2018
@sadielbartholomew sadielbartholomew self-assigned this Nov 13, 2018
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bugs all replicated, fixes all tested. Good!

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides PyCharm and Eclipse, the only other editor I normally use is vim. I think I used emacs in the university to follow the assignments of a professor... but unfortunately I never had a chance to play with emacs again. So take my review with a pinch of salt 😬

Regexes look OK (I can normally parse/understand some regex-fu, but normally rely on IDE + Pythex to tell me if I messed it up).

Installed emacs25 on Ubuntu, followed instructions to set up the syntax highlighting (slightly broken I think, but sending a pull request for that), and can confirm that at least the task names are now matching before/after the => token.

Here's the screen shot of emacs with the change in this pull request.

capture

And here's what it was looking before. Notice that the task on the left has a different colour.

capture2

+1 LGTM

@kinow kinow merged commit 36a9a05 into cylc:master Nov 15, 2018
@sadielbartholomew
Copy link
Collaborator Author

the only other editor I normally use is vim

Thanks @kinow. Sorry for having to assign you as second reviewer, a stand-in was needed for Hilary who reviewed the original PR, though being an emacs user is not strictly necessary as basic inspection & typing should suffice (in fact both he & Oliver also use vim).

@sadielbartholomew sadielbartholomew deleted the emacs-lexer-slowness-fix branch November 15, 2018 14:55
@kinow
Copy link
Member

kinow commented Nov 15, 2018

It is actually good so I can learn some emacs too. Happy to review future issues like this and keep learning :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants