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

Fix various oversights in modeline regexes #5271

Merged
merged 5 commits into from
Mar 19, 2021
Merged

Fix various oversights in modeline regexes #5271

merged 5 commits into from
Mar 19, 2021

Conversation

Alhadis
Copy link
Collaborator

@Alhadis Alhadis commented Mar 12, 2021

This PR fixes a handful of oversights and behavioural discrepancies with the regular expressions used by our modeline strategy (detailed below).

Description

Whilst reviewing #4138, I recommended [ \t] be used instead of [:blank:]. However, I didn't think to check if the submitter applied the changes correctly. Only recently did I notice Vim's modeline regex had an erroneous [^\\[ \t]] (which should have been [^\\ \t] instead).

Realising there might be other things I missed, I recently took the time to double-check both regexes and verify their behaviour matches that of Emacs and Vim. Among the things I found were:

  • Vim's modelines are case-sensitive.
    :help modeline says that modelines must be prefixed by “the string vi:, vim:, Vim: or ex:. Mixed-case prefixes like vIM: or ViM: don't work, so VIM_MODELINE shouldn't be flagged as case-insensitive.

  • Versioned modelines must start with Vim or vim.
    Our current pattern allows vi>600: ft=ruby and # ex>600: ft=ruby, neither of which actually work in Vim.

  • Emacs's modeline pattern allowed newlines.
    This was due to \s being used instead of [ \t]. An embarrassing habit I've picked up from years of writing TextMate grammars is to assume \s only matches horizontal whitespace, instead of line-breaks as well. Ergo, something like this (obviously) shouldn't match:
    -*-
    markdown
    -*-
    

  • Modeline mode wasn't disabled.
    So Ruby's insidious treatment of . meant that .*? also matched newlines, which it logically shouldn't. I resolved this by unsetting multiline mode with (?-m).

  • Carriage returns were allowed inside vim: set … :.
    A trivial issue that probably won't ever surface in practice; fixed by updating classes to use \r\n instead of just \n.

(Template removed as it doesn't apply)

@Alhadis Alhadis requested a review from a team as a code owner March 12, 2021 07:30
Alhadis added a commit to file-icons/atom that referenced this pull request Mar 12, 2021
The capitalisation of "prolog" is left intact to assert case-insensitive
comparison of language names (i.e., we don't care that Vim's Prolog mode
is actually lowercase).
Alhadis added a commit to Alhadis/YASR that referenced this pull request Mar 12, 2021
Alhadis added a commit to Alhadis/language-etc that referenced this pull request Mar 12, 2021
Also, fix a copy+paste error in the gitattributes(5) grammar.
Alhadis added a commit to Alhadis/language-grammars that referenced this pull request Mar 12, 2021
Alhadis added a commit to Alhadis/language-fontforge that referenced this pull request Mar 12, 2021
Alhadis added a commit to Alhadis/language-viml that referenced this pull request Mar 12, 2021
Alhadis added a commit to Alhadis/language-gn that referenced this pull request Mar 12, 2021
Alhadis added a commit to Alhadis/language-apl that referenced this pull request Mar 12, 2021
Alhadis added a commit to Alhadis/language-roff that referenced this pull request Mar 13, 2021
Alhadis added a commit to Alhadis/language-emacs-lisp that referenced this pull request Mar 13, 2021
Alhadis added a commit to Alhadis/language-webassembly that referenced this pull request Mar 13, 2021
Alhadis added a commit to Alhadis/language-sed that referenced this pull request Mar 13, 2021
Alhadis added a commit to Alhadis/language-texinfo that referenced this pull request Mar 13, 2021
Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

Nice 😍 I'm surprised we've gone for so long without anyone noticing 🤭

@Alhadis Alhadis merged commit 2a6f9e2 into master Mar 19, 2021
@Alhadis Alhadis deleted the regex-fixes branch March 19, 2021 10:54
Alhadis added a commit to Lukasa/language-restructuredtext that referenced this pull request Aug 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants