-
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
Replace Genero language syntax files with official files #6629
Comments
Closing as a dupe of #6628 |
Oh wait. You mean the grammar files. Re-opening, but note your repo is not accessible. |
@lildude I have just started to work on these and doing github forking and pull requests, so please forgive my mistakes. |
I am good with my version of the grammars being replaced with the official grammars maintained by the author(s) of the language. Thank you again @sebflaesch! |
https://github.com/FourjsGenero/GeneroFgl.tmbundle is now public. |
I think I need some help on this: The new TextMate files (https://github.com/FourjsGenero/GeneroFgl.tmbundle) we would like to be used by Linguist/Github are defining new language names and identifiers (for the two distinct file types .4gl and .per), that do not match the names and ids that are in place. Consequently, I guess that a simple "--replace" option with script/add-grammar would not work. In such case, what is the best practice? Keep in mind that the author of the current definition (@alienriver49) is one of our customers and is ok to replace all of the existing definitions by ours. In fact, we have currently two "language" definitions:
It is possible to do a complete drop of the existing definitions, and add again the new languages definitions with the script/add-grammar command? I could not find the instructions to drop a definition, in https://github.com/github-linguist/linguist/blob/master/CONTRIBUTING.md. Regarding samples (issue #6628), my understanding is that the directory names under TOP/samples must match the language names, correct? If yes, we should provide a single pull request for both #6628 and #6629. And we should certainly find a better name for the .per files, because non of the existing language names I can see use ( ) parentheses... |
Seems that the support for Genero language was added in a single commit: commit adc5319 So if this works it would not be a big deal to revert this commit, and add a new language definition for Genero... |
It most definitely does work 😁 If you stick with the instructions for replacing a grammar in the CONTRIBUTING.md file as they're written you should be good to go. You shouldn't need to do anything that isn't documented in that section. Use the path to the current vendor directory (not path) for the So if you start with the "Genero" grammar, you'd run You won't need to do anything for the "Genero Forms" grammar as the script will detect the same grammar is now used for both languages and update the relevant files. As for dropping the current definitions: don't. You can certainly rename them, but don't change the ID. This is stored and heavily cached in a lot of locations outside of the main GitHub database and is not an easy task to change or correct. Many third parties rely on this ID too. As such, once an ID is set, it is never changed and a language is never removed. As for the language names: I'd advise against using parentheses (it should work for GitHub, but we can't guarantee elsewhere) and use the names users are most likely to know and use. For most people this is the commerical name of the language. |
By ID do you mean the "language_id" field in lib/linguist/languages.yml, that is produced by the script/update-ids? For now, in my local fork, I could manage to revert the original commit, setup the ruby/docker env and add from scratch the new languages "Genero 4gl" (instead of "Genero") and "Genero per" (instead of "Genero Forms"), with the regular script/add-grammar process, and created new IDs with script/update-ids ... Would it be an issue, if I reuse the IDs ("language_id") that where generated by the first commit, but using the new names "Genero 4gl" / "Genero per" of my new adding? This would save me some more additional work, as the only change I would have to do is:
Seb |
Yes.
There's no need to revert the original commit; it's really not necessary. Follow the steps for replacing the grammar, commit those changes, rename the entries as you wish, commit those changes, rename the samples directories to match the new language names, commit your changes, update the Several files will be changed but the only changes in the I will reject the PR if you change the IDs as I said before: these must not change. |
Ok no worries: I clearly understood that the language IDs must NOT change. |
So I followed: https://github.com/github-linguist/linguist/blob/master/CONTRIBUTING.md#changing-the-source-of-a-syntax-highlighting-grammar:
What's next? I thought I should patch the tm_scope identifiers, to match the files of the new TM repo, but that did not help (I really wonder how the "replace" process can guess what is to be replaced if these are different!)
|
I gave you the exact command to run earlier 😉 This won’t update languages.yml but will take care of everything else. |
Start the PR and we can continue this there as it’s best suited in the PR. |
Oups ok Colin, using
worked. Before doing any PR, I can share the testing fork I am working on, if you like: I need also to sync with the author of https://github.com/FourjsGenero/GeneroFgl.tmbundle Thanks for your assistance! |
@lildude : I have worked on changes, but this is only for validation! please do not apply! Would you mind check the changes I have prepared and tell me if something is missing? Note that there is NO license file (our TM repo has "The Unlicense") - is this an issue? Should it be an MIT license? |
Diff looks good from a quick glance.
We need a license file, even for the The Unlicense. The text from https://choosealicense.com/licenses/unlicense/ in a LICENSE file will do the trick. |
Sorry I was not clear: We have that "unlicense" text: |
* chore: add Glimmer JS to languages * chore: add vsc-ember-syntax grammar * chore: add Glimmer JS samples * chore: run script/update-ids * fix: run script/sort-submodules * chore: pull updated vsc-ember-syntax submodule * chore: run script/add-grammar --replace vsc-ember-syntax https://github.com/lifeart/vsc-ember-syntax * fix: run script/sort-submodules
@lildude : How can I check that my local fork of linguist detects the new "Genero 4gl" and "Genero per" languages replacing the existing languages "Genero" and "Genero Forms"? When using the github-linguist command, I still see the old languages... What I am missing here?
Is this because I am on a branch (issue-6629) of my repo that is a fork of linguist?
I have also tried git-linguist command:
|
@sebflaesch It looks like you've not updated the names in the |
@lildude quite confusing... |
Nope. That script only changes the grammar submodule and related vendor/README.md and cached license files. That's why I said:
It doesn't matter. It only needs to be done once. |
OK I did the renaming in languages.yml, I have pushed it to my fortk/branch and the change is visible here: Now when I run the github-linguist command, how can I see my "Genero 4gl" and "Genero per" languages?
My guess is that it shows only the top most matching languages, but obviously, in this linguist repo, there are very few .4gl and .per sources so it would be something like 0.01% or less... I have also tried:
I have executed the tests:
|
"Genero 4gl" / "Genero per" does not show up when running:
|
Please open a PR. This discussion should be happening in the PR, not the issue. It's also a lot easier to see and also gives us constant confirmation of behaviour via the tests. |
This is because Linguist does not count the linguist/lib/linguist/documentation.yml Lines 33 to 34 in 6a9a3e4
You need to fetch the |
No probs. Noisy PRs are fine as this is where all the dev work is taking place. Context switching between issue and code makes this a lot harder to track. |
Changes have shipped. Closing. |
@lildude : Seems that something is not working... |
No need. This is because the language has been renamed but the cache not invalidated. We don't invalidate the cache when making changes via Linguist as this would be incredibly resource intensive given the number of repositories involved. Pushing a new change will force a re-analysis and things should reflect the new name. You can see the result of this in my fork (I'll delete this later) in which I edited the README to force a reanalysis: |
ok got it! thanks a lot! |
Any trick to force a Linguist re-analysis of sources without pushing changes into a given repo? |
Nope. This is by design as per the same reasoning for not invalidating the caches. |
New official syntax language files provided by FourJs are available here:
https://github.com/FourjsGenero/GeneroFgl.tmbundle
We need to replace the existing files created by hand by Bryan Jackson (@alienriver49) in 2020.
Bryan, we discussed this today and I thank you again for your valued contribution.
Please just confirm here that you accept that we replace your original syntax files with ours.
Cheers!
Seb
The text was updated successfully, but these errors were encountered: