Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Add support for Emacs/Vim modelines #420

Merged
merged 5 commits into from Sep 12, 2016
Merged

Add support for Emacs/Vim modelines #420

merged 5 commits into from Sep 12, 2016

Conversation

Alhadis
Copy link
Contributor

@Alhadis Alhadis commented Aug 29, 2016

Self-explanatory; this PR associates JavaScript with any files that contain an Emacs or Vim-style modeline in their header:

// -*- js -*-
// -*- JavaScript -*-
/* vim: ft=javascript */

@jerone
Copy link

jerone commented Aug 29, 2016

Correct me if I'm wrong but Emac and Vim aren't part of the JavaScript (EcmaScript) specs, so why add them here and not in a separate language (inheriting from this package maybe)?

@Alhadis
Copy link
Contributor Author

Alhadis commented Aug 29, 2016

Good idea. Should we drop support for interpreter directives like #!/usr/bin/node too, then?

@50Wliu
Copy link
Contributor

50Wliu commented Aug 29, 2016

The firstLineMatch is intended for exactly things like this. It's meant as an alternative to the fileTypes array. Linguist, for example, also tries to match files based on the modeline.

That said, I'm not a fan of all the extra fixtures. It should be possible to inline all of those, right?

@Alhadis
Copy link
Contributor Author

Alhadis commented Aug 29, 2016

Not that I know of. The matching is done when a file is opened, which means the only alternative (that I know of) would be to automatically generate the fixtures when running specs, which would just complicate stuff.

@Alhadis
Copy link
Contributor Author

Alhadis commented Sep 5, 2016

@50Wliu I'm an idiot, I have no idea why using the grammar's firstLineRegex never occurred to me.

Fixtures removed and remedied.

@50Wliu
Copy link
Contributor

50Wliu commented Sep 5, 2016

What in the world happened to Coffeescript's multiline quotes?

@Alhadis
Copy link
Contributor Author

Alhadis commented Sep 5, 2016

You referring to the error highlighting in this PR's diff?

Yeah, not really sure what's up with that. :S It's perfectly fine in Atom.

@50Wliu
Copy link
Contributor

50Wliu commented Sep 5, 2016

Going to let this sit for a bit longer in case someone spots some issues with the modelines (I've never used Emacs or Vim). Remind me about this in a week please?

@Alhadis
Copy link
Contributor Author

Alhadis commented Sep 5, 2016

As you wish.

The existing code doesn't take into account some less obvious cases:

    #!/bin/env --type=node
    -*- not-mode: js -*-
    vim: noexpandtab: ft=javascript
@Alhadis
Copy link
Contributor Author

Alhadis commented Sep 12, 2016

... wait, did you just say you've never used Emacs or Vim?!

ABSOLUTELY DISGUSTING

... also, here's your weekly reminder. =)

@50Wliu
Copy link
Contributor

50Wliu commented Sep 12, 2016

I'm liking this test coverage.

@50Wliu 50Wliu merged commit b3d2bb6 into atom:master Sep 12, 2016
@Alhadis Alhadis deleted the modelines branch September 12, 2016 15:04
@Alhadis
Copy link
Contributor Author

Alhadis commented Sep 29, 2016

What in the world happened to Coffeescript's multiline quotes?

Heh, I figured it out. The modelines are telling GitHub it's a JavaScript file, so it's using the JS grammar for highlighting. It's doing the same for the other spec-files too. :S

Should I prepend a real modeline at the start of each relevant suite to tell Linguist it's really a CoffeeScript file? I mean, the incorrectly-highlighted comments are one thing, but this is really another.

@50Wliu
Copy link
Contributor

50Wliu commented Sep 29, 2016

Yeah, I was laughing at that one. But really, modelines should be at the start of the file, not somewhere random in the middle (and in quotes!)...would that count as a Linguist bug?

@Alhadis
Copy link
Contributor Author

Alhadis commented Sep 29, 2016

Nope, modelines are often placed at the bottom of a file too, which is the case with all of Vim's syntax files, for example. Linguist reads the first and last five lines when checking for modelines. It's obviously being fooled by the last few Vim modelines, because they're using double-escapes.

We could rearrange the lines so the last two aren't within the 5-line radius Linguist uses to scan for modelines, but I think the more sustainable approach is to simply include an extra line at the bottom instead. The scan-scope is, after all, subject to change... especially now that the modeline-patterns have been reinforced to fix the errors which the scope-reduction was intended to ameliorate.

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

Successfully merging this pull request may close these issues.

None yet

3 participants