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 mcfunction language #4425

Merged
merged 8 commits into from
Mar 1, 2019
Merged

Add mcfunction language #4425

merged 8 commits into from
Mar 1, 2019

Conversation

Arcensoth
Copy link
Contributor

@Arcensoth Arcensoth commented Feb 17, 2019

Description

This is an interesting case of "language" that originates from Minecraft server commands. The mcfunction file format is essentially a collection of server commands, one per line, with additional comment support and some unique runtime properties. The idea is simple, but the underlying command system is complex enough to warrant discussion about whether mcfunction should be considered a type of programming language. In any case: mcfunction files are both written and distributed in plain text, naturally giving way to open-source collaboration.

The search results listed in the checklist provide proof that mcfunction is being used across a reasonable number of repositories. There's even an open-source language server in development that includes extensive support for the mcfunction format.

I ran linguist dev on the language-mcfunction repository to get a breakdown of the results and to ensure my changes are functional.

Checklist:

@Arcensoth
Copy link
Contributor Author

Arcensoth commented Feb 17, 2019

Not sure why the build "errored" as opposed to failing or passing. Looks like it got part way through script/travis/before_install and then died for no apparent reason:

https://travis-ci.org/github/linguist/jobs/494397050#L1494

(Edit) On a second glance, I saw this:

https://travis-ci.org/github/linguist/jobs/494397050#L1213

Error updating vendor/grammars/language-mcfunction
error: pathspec 'vendor/grammars/language-mcfunction' did not match any file(s) known to git.

Did I miss something in the commit after the script/add-grammar step?

@pchaigno
Copy link
Contributor

Yes, there's something missing. Did you use the provided script to add the grammar as per the contribution guidelines? Did you get any error message?

@Arcensoth
Copy link
Contributor Author

Yes I followed the contribution guidelines thoroughly. I ran the script, and it was successful. I was able to run linguist and get a breakdown afterwards.

I cross-referenced with some other language PR's (here and here) and couldn't find any evidence of what's missing here.

@pchaigno
Copy link
Contributor

Maybe you forgot to commit the submodule itself then. It should be under vendor/grammars/name of the grammar repo.

@Arcensoth
Copy link
Contributor Author

Arcensoth commented Feb 17, 2019

Nice catch. But because language-mcfunction has received commits since I submitted the PR, I'm now facing a problem where the cached license is outdated. And apparently the language is not sorted correctly, but IIRC it was a script that added re-ordered it. (?) Anyway, I'll fix and try again.

@Arcensoth
Copy link
Contributor Author

This may or may not be worth consideration, but there is an inconsistency between vendor/README.md and lib/linguist/languages.yml regarding how languages are sorted by name. As far as I can tell, the latter sorts upper-case first whereas the latter disregards case and mixes them together.

@pchaigno
Copy link
Contributor

As far as I can tell, the latter sorts upper-case first whereas the latter disregards case and mixes them together.

Indeed. @Alhadis What do you think? Let's sort case-sensitively?

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.

Thanks for your contribution and for the neat pull request @Arcensoth!

@Arcensoth
Copy link
Contributor Author

Thanks for reviewing!

Small suggestion for CONTRIBUTING.md: add a step for Testing under the Adding an extension to a language and sections Adding a language. I think it could reduce the number of questions as a result of Travis errors, and also encourage contributors to submit cleaner PRs. :)

@Alhadis
Copy link
Collaborator

Alhadis commented Feb 18, 2019

@Alhadis What do you think? Let's sort case-sensitively?

Sorry, I missed this. Yes, let's. Case-sensitive is strange and feels very machine-line.

@lildude lildude merged commit 849b5dc into github-linguist:master Mar 1, 2019
@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.

None yet

4 participants