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 new entry for Sublime Text config files #3268

Merged
merged 6 commits into from
Oct 10, 2016
Merged

Add new entry for Sublime Text config files #3268

merged 6 commits into from
Oct 10, 2016

Conversation

Alhadis
Copy link
Collaborator

@Alhadis Alhadis commented Oct 6, 2016

Follow-up of #3267 and #2662, both of which concern the misclassification of Sublime Text's configuration files as "JavaScript". These files are most certainly not JavaScript... the only thing distinguishing them from JSON is support for JS comments.

Because many of them do contain comments, using the JSON grammar to provide highlighting will produce a lot of incorrect, messy highlighting. I've added a separate entry to group them under JSON, but use the JavaScript grammar for highlighting.

Linguist's submodules have a healthy abundance of Sublime configs already, so the samples I added were simply copied from vendor/grammars.

With @keplersj's help, I was able to collate a list of normative references for each documented format. Some were undocumented altogether; presumably deprecated filenames supported in older versions.

EDIT: Relevant PR for CSON that still needs reviewing

File extension Format Results: 358,824
.sublime-build JSON + Comments 11,601
.sublime-commands JSON + Comments 25,177
.sublime-completions JSON 9,659
.sublime-keymap JSON 56,300
.sublime-macro JSON 8,639
.sublime-menu JSON + Comments 38,843
.sublime-mousemap JSON - Undocumented 5,479
.sublime-project JSON 53,241
.sublime-settings JSON 98,640
.sublime-theme JSON - Undocumented 5,896
.sublime-workspace JSON 44,000
.sublime_metrics JSON - Undocumented 121
.sublime_session JSON - Undocumented 1,228

@keplersj
Copy link
Contributor

keplersj commented Oct 6, 2016

I'd be interested in knowing how many of these files actually have comments in them. I still suggest using the JSON 5 syntax highlighter instead of the JSON highlighter just because the former supports comment and the latter does not.

@keplersj
Copy link
Contributor

keplersj commented Oct 6, 2016

As well, if my math follows about 1.5% of files identified as JavaScript on GitHub are these Sublime config files.

@Alhadis
Copy link
Collaborator Author

Alhadis commented Oct 6, 2016

I'd be interested in knowing how many of these files actually have comments in them. I still suggest using the JSON 5 syntax highlighter instead of the JSON highlighter just because the former supports comment and the latter does not.

Ah, THAT would be the reason these files were categorised as JavaScript in the first place.

Right, TMTOWTDI. @arfon, if it's alright, I'm going to define a new entry just for these Sublime config files... I'm sure many users would complain about the sudden lack of comment highlighting, and I don't want these things continuing to skew stats.

@Alhadis Alhadis changed the title Reclassify Sublime configuration files as JSON Add new entry for Sublime Text config files Oct 7, 2016
@Alhadis
Copy link
Collaborator Author

Alhadis commented Oct 7, 2016

Alright, it's done. Only one grammar can be assigned per language, which is why a new entry is necessary.

To play it safe, I decided to keep using the JavaScript grammar, rather than JSON5's grammar. It feels more future-proof.

@arfon
Copy link
Contributor

arfon commented Oct 9, 2016

👍 looks good. Did we (I mean you @Alhadis 😁) update the language_id appropriately for this new language?

@Alhadis
Copy link
Collaborator Author

Alhadis commented Oct 10, 2016

We/I did! =) It was done by hand though... let me know if there was something else I should've done.

Sorry, I would've answered this 10 hours earlier had the entire region not been without electricity yesterday. ⚡️

@arfon
Copy link
Contributor

arfon commented Oct 10, 2016

@Alhadis - because of this PR I think we'll need to bump this now.

We/I did! =) It was done by hand though... let me know if there was something else I should've done.

There's set-language-ids --update which will set the value if there isn't one present. This script could do with some TLC though as the update process is less than ideal.

@pchaigno
Copy link
Contributor

There's set-language-ids --update which will set the value if there isn't one present. This script could do with some TLC though as the update process is less than ideal.

Shouldn't be a problem once we start using hash ids ;)

@Alhadis
Copy link
Collaborator Author

Alhadis commented Oct 10, 2016

@Alhadis - because of this PR I think we'll need to bump this now.

Haha, yeah, I figured there'd be a race involved between these two PRs. All the more reason to use hashed IDs. =)

Done.

@arfon arfon merged commit 22c2cf4 into github-linguist:master Oct 10, 2016
@arfon
Copy link
Contributor

arfon commented Oct 10, 2016

Haha, yeah, I figured there'd be a race involved between these two PRs. All the more reason to use hashed IDs. =)

😆 yes.

@Alhadis Alhadis deleted the sublime branch October 11, 2016 00:01
@Alhadis
Copy link
Collaborator Author

Alhadis commented Oct 11, 2016

@arfon Any feedback on this? This is arguably skewing statistics much worse than the Sublime config files were.

@Alhadis Alhadis mentioned this pull request May 31, 2018
4 tasks
@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jun 17, 2024
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.

4 participants