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

Fix Nixinova submodule issues #5292

Merged
merged 1 commit into from
Apr 6, 2021

Conversation

am11
Copy link
Contributor

@am11 am11 commented Mar 27, 2021

Description

Nixinova submodule was manually added without using script, which results in it getting deleted from .gitmodules and grammers.yml files when other languages are added or deleted

Also added a second positional argument in script/add-grammer, to override the repository name. In this case repo name is tmLanguage and the intended name is Nixinova-tmLanguage.

Exact command used (after deinit the exiting submodule):

$ script/add-grammar https://github.com/Nixinova/tmLanguage Nixinova-tmLanguage

Fixes #5291

Checklist:

  • I am updating a grammar submodule

@am11 am11 requested a review from a team as a code owner March 27, 2021 02:36
@Nixinova
Copy link
Contributor

A more descriptive title for this PR would be Add optional output folder argument to script/add-grammar.

@Alhadis
Copy link
Collaborator

Alhadis commented Mar 27, 2021

Also added a second positional argument in script/add-grammer, to override the repository name. In this case repo name is tmLanguage and the intended name is Nixinova-tmLanguage.

Erh, why not rename your repository to nixinova-linguist-grammars instead? Or nixinova-grammars, nixinova-textmate, etc… Moreover, this would break consistency between a repository's name and its submodule's name.

@am11
Copy link
Contributor Author

am11 commented Mar 27, 2021

why not rename your repository to nixinova-linguist-grammars instead?

That's what I suggested to him in the original issue, twice.

A more descriptive title for this PR would be

Not really, as I mentioned in the issue and in the description, the main problem with current master branch is that when someone tries to add a new grammar, it deletes your grammar because it was not added properly using script/add-grammar script.

When we do try to add your grammar using script/add-grammar script, the first error we hit is because the sub-submodule in your repo was pointing to wrong URL which was fixed by Nixinova/NovaGrammars#2. The next issue was linguist compiler gives 10+ fatal errors which were fixed by Nixinova/NovaGrammars#4. Now the upstream PRs are merged, this PR is potentially updating the submodule to fix the repo state. So the title makes sense to me.

@Nixinova
Copy link
Contributor

this would break consistency between a repository's name and its submodule's name.

Well, there aren't any other repos in this called tmLanguage so just leaving this as-is would work.

@lildude
Copy link
Member

lildude commented Mar 31, 2021

Thanks for this @am11. As with @Alhadis, I'm not keen on allowing peeps to break the consistency between a repository's name and its submodule name as used by Linguist as it adds unnecessary complexity when trying to track problems down.

Well, there aren't any other repos in this called tmLanguage so just leaving this as-is would work.

Indeed, however I'd much rather have you rename to a less-generic name which has less risk of clashing or causing confusion.

@am11
Copy link
Contributor Author

am11 commented Mar 31, 2021

@lildude, agreed; that was my first preference as well. I will update the PR when the repo is renamed.

@Nixinova
Copy link
Contributor

Okay, fine, renamed my repo

Copy link
Contributor

@Nixinova Nixinova left a comment

Choose a reason for hiding this comment

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

I had changed the repo name again to Nova.tmLanguage while you were doing this.

@am11
Copy link
Contributor Author

am11 commented Apr 1, 2021

I had changed the repo name again to Nova.tmLanguage while you were doing this.

or

I changed my mind (at least) one hour later and renamed again

otherwise timestamps and passing CI make no sense

@Nixinova
Copy link
Contributor

Nixinova commented Apr 1, 2021

It was changed literally 10sec later. See above, at how Nixinova/NovaGrammars#5 is located above your commit.

@am11
Copy link
Contributor Author

am11 commented Apr 1, 2021

It was changed literally 10sec later. See above, at how Nixinova/Nova.tmLanguage#5 is located above your commit.

image

@Nixinova
Copy link
Contributor

Nixinova commented Apr 1, 2021

Not sure why you deleted your old comment as that would've been what happened.

@am11
Copy link
Contributor Author

am11 commented Apr 1, 2021

It was not relevant to your 10sec comment, which I read after the page refresh. The difference was one hour not 10 sec.

@lildude
Copy link
Member

lildude commented Apr 6, 2021

@am11 would you mind updating this PR for the final new repo name?

@am11 am11 force-pushed the feature/submodule/nixinova branch 2 times, most recently from 8d4d02e to eb40644 Compare April 6, 2021 09:37
@lildude
Copy link
Member

lildude commented Apr 6, 2021

I think you need to pull master into your branch as you appear to be missing the NimLime grammar change just merged.

@am11 am11 force-pushed the feature/submodule/nixinova branch from eb40644 to aca29cf Compare April 6, 2021 10:05
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.

Lovely! Thanks all. 🙇

@lildude lildude merged commit 79b3da8 into github-linguist:master Apr 6, 2021
@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.

Issues with script/add-grammar
4 participants