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

Add syntax highlighting for .nanorc files and Git-related dotfiles #4343

Merged
merged 12 commits into from
Dec 18, 2018

Conversation

Alhadis
Copy link
Collaborator

@Alhadis Alhadis commented Dec 6, 2018

Fixes #4335 by adding a bespoke highlighting grammar for .nanorc files, as well as several extra config formats:

  • .gitignore (and ignore-type manifests based on its syntax; e.g., .npmignore, .dockerignore, etc)
  • .gitconfig, .gitmodules
  • .gitattributes

Description

Since we're adding support for a language solely on aesthetic grounds, I thought to add highlighting for a few other widespread (and obvious) config formats: those used by Git itself.

Even if this addition is strictly superficial, I expect it'll be noticed (and welcomed) by many.

You'll notice .gitignore has been listed as "Ignore List". There are so many analogous formats based upon the gitignore(5) syntax, or are similar enough that they're practically interchangeable (e.g., .bzrignore, .cvsignore).

There are probably countless other ignore-like formats with enough adoption to qualify for registration, but I've only added those I'm aware of (...which was exhausting enough already).

Checklist:

@Alhadis Alhadis requested a review from pchaigno December 6, 2018 06:57
@Alhadis
Copy link
Collaborator Author

Alhadis commented Dec 6, 2018

@vmg, @lildude Would one of you care to test this with the grammar-compiler, please? 😬 I, erm, had to fudge docker with a blank executable in my $PATH because there's no Docker port for OpenBSD, and RubyGems hates each and every single MacBook I touch. 😥 That is, I can never get Linguist setup locally because I'm a r00b (Ruby n00b).

@lildude
Copy link
Member

lildude commented Dec 6, 2018

@Alhadis

$ script/grammar-compiler add vendor/grammars/language-etc
  > latest: Pulling from linguist/grammar-compiler
  > Digest: sha256:159ff655c832de0e3ab3ea1366992606b5f604bcb64f8757b6c575d4a7628bc6
  > Status: Image is up to date for linguist/grammar-compiler:latest
  > The new grammar repository `vendor/grammars/language-etc` (from https://github.com/Alhadis/language-etc) contains 1 errors:
  >     - Unknown keys in grammar: `source.nanorc` (in `grammars/nanorc.cson`) contains invalid keys (`maxTokensPerLine`)
  >
  > failed to compile the given grammar
$

@Alhadis
Copy link
Collaborator Author

Alhadis commented Dec 6, 2018

That should be whitelisted. The field is supported by Atom, although it doesn't appear to have any impact on Lightshow when I tried it... 😞

@pchaigno
Copy link
Contributor

pchaigno commented Dec 6, 2018

That should be whitelisted.

@Alhadis You can add it to data.go then.

Copy link
Contributor

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

That's great! We should make a bit of noise to let peeps know you did this!

I have a couple comments on usage below.

lib/linguist/languages.yml Outdated Show resolved Hide resolved
lib/linguist/languages.yml Show resolved Hide resolved
lib/linguist/languages.yml Show resolved Hide resolved
@Alhadis
Copy link
Collaborator Author

Alhadis commented Dec 6, 2018

@Alhadis You can add it to data.go then.

Done and done! 😀 I'll make it a part of this PR, since there's no point slowing everything down by submitting a separate pull-request...

Copy link
Contributor

@vmg vmg left a comment

Choose a reason for hiding this comment

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

Sensible addition! Let's see what @lildude says.

@lildude
Copy link
Member

lildude commented Dec 14, 2018

@Alhadis You can add it to data.go then.

Done and done! 😀 I'll make it a part of this PR, since there's no point slowing everything down by submitting a separate pull-request...

I'm clearly doing something wrong (complete Docker and Go noob) but I can't get this change to be picked up. 😞

@Alhadis
Copy link
Collaborator Author

Alhadis commented Dec 14, 2018

Did you rm -rf the contents of the tmp/ directory? (Relative to your linguist checkout, I mean, not the /tmp folder!)

@lildude
Copy link
Member

lildude commented Dec 14, 2018

Did you rm -rf the contents of the tmp/ directory?

Yup. Whipped out the big docker image rm hammer too, and still no luck.

@Alhadis
Copy link
Collaborator Author

Alhadis commented Dec 14, 2018

😨 Okay, erm, I'll leave this up to @vmg to figure out. 😰

@lildude
Copy link
Member

lildude commented Dec 14, 2018

Woohooo!!! Worked it out.

This bad boy is to blame 🤦‍♂️ :

https://github.com/github/linguist/blob/fa493000a594f5fbd457bbd473ce791d95b227cc/script/grammar-compiler#L9

Comment it out and 🎉

$ script/grammar-compiler add vendor/grammars/language-etc
OK! added grammar source 'vendor/grammars/language-etc'
	new scope: source.curlrc
	new scope: source.gitattributes
	new scope: source.gitconfig
	new scope: source.gitignore
	new scope: source.hgignore
	new scope: source.m4
	new scope: source.nanorc
$

Oh yes, I needed to rebuild the image too.

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.

I've got no objections with this if @pchaigno doesn't.

@Alhadis
Copy link
Collaborator Author

Alhadis commented Dec 14, 2018

I feel like I'm to blame with my hacky docker spoofing. 😅 Apologies for the mess and confusion.

@Alhadis
Copy link
Collaborator Author

Alhadis commented Dec 18, 2018

I've got no objections with this if @pchaigno doesn't.

Ahem. @pchaigno, that would be your cue for the final response. 😉

@pchaigno
Copy link
Contributor

@Alhadis There's one unresolved discussion above: I think we should remove the gitignore filename.

@Alhadis
Copy link
Collaborator Author

Alhadis commented Dec 18, 2018

Burnt it.

@Alhadis Alhadis merged commit 36f4592 into master Dec 18, 2018
@Alhadis Alhadis deleted the dotfile-highlighting branch December 18, 2018 07:25
@Alhadis
Copy link
Collaborator Author

Alhadis commented Dec 18, 2018

More dotfile highlighting to come in future the next time I'm stabbed by an impulsive urge to work on some random grammar at 4:45 am. =)

@perlun
Copy link

perlun commented Dec 20, 2018

@Alhadis 😄 Any idea of when this will be deployed to GitHub?

@Alhadis
Copy link
Collaborator Author

Alhadis commented Dec 20, 2018

@perlun Updates to Linguist become live on GitHub once a new release of Linguist has been cut. The release cycle isn't set in stone, but it generally happens roughly once per month.

@perlun
Copy link

perlun commented Dec 21, 2018

@Alhadis Alright, I see. I assumed it was being continuously delivered. 😉

@Alhadis
Copy link
Collaborator Author

Alhadis commented Dec 21, 2018

Well, it is in a way — every change made to the repository's master branch triggers a new job in CI, which alerts contributors to unexpected build failures. But that's a different affair to actually deploying code live on GitHub.

@perlun
Copy link

perlun commented Dec 21, 2018

But that's a different affair to actually deploying code live on GitHub.

Yeah, I know. I was just trying to be funny. 😄

Anyway, thanks for the change and have a Merry Christmas soon there in the "land down under"! 🐊 🏄 etc. 😃

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

Successfully merging this pull request may close these issues.

Add support for nanorc
5 participants