-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
New languages: Modula-3 and Quake #4139
Conversation
The CI test failed because “cached license data” was missing.
Am I supposed to add these files manually? @lildude |
The license should have automatically put in the right place when you ran |
Unfortunately you can’t make that assumption. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request!
I've got a single comment below. LGTM otherwise!
lib/linguist/languages.yml
Outdated
- m3makefile | ||
- m3overrides | ||
extensions: | ||
- ".tmpl" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original post is missing a search link for this extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll add any missing info later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a search link for .tmpl files, as well as other previously missing info. @pchaigno
I’ll take a closer look later, but from a quick glance several of these extensions don’t appear to meet the minimum “hundreds of repos” popularity requirement at the moment. |
Let’s resolve this “missing cached license” issue first. I tried to run add-grammar again, but the license was still missing.
Or is there something wrong with https://github.com/newgrammars/m3 or https://github.com/newgrammars/quake? |
Ooof. Looks like Licensed 1.0.1 breaks things for us:
If I force the use of Licensed 1.0.0:
... and try again, it works...
@jonabc Any idea what's going on here? My suspicion is github/licensed#21 may be related. |
How about I handwrite m3.txt and quake.txt and check them in now? |
@lildude thanks for the report! It looks like there's two things going on here
This is related to the PR you linked in
This is also related to the PR you linked and will be fixed to force an evaluation if the current version string is nil. On the linguist side, it might be good if linguist provided version metadata for each grammar's license content. In similar situations I've used the most recent commit SHA for a given folder as the version string. This would allow licensed to know if/when the license data has changed. It would also add chattiness around version updates in the license files as submodules are synced and SHAs updated. Completely up to you whether the cost/benefit makes sense. |
Interesting idea. Lets discuss this outside the context of this PR. |
Is there any other missing information? @pchaigno |
According to this search query, the |
Unfortunately, the 14 keywords and those of the CM3-specific builtins that are really used in most makefiles (like Realistically, though, the number of Quake files isn’t going to be in the same order of magnitude as Modula-3 source files. Each CM3 program or library exists in a package—see any folder here for an example—which may have just a single “m3makefile”. |
The test failures appear to be caused by recent changes to licensee which has resulted in a slightly modified hash for the four licenses in question. I'll address this in #4152. |
Merging master into your branch should fix the failing tests. |
@lildude This looks good to go! |
This commits adds new language support for Modula-3 and Quake, a small scripting language primarily used for writing makefiles for Modula-3 projects.
Description
Please see issue #4136 for details.
Checklist:
I can’t be sure just yet. But since these files are published on GitHub they should have been permissibly licensed.