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 script to alphabetise submodule list #4054

Merged
merged 1 commit into from
Mar 2, 2018
Merged

Add script to alphabetise submodule list #4054

merged 1 commit into from
Mar 2, 2018

Conversation

Alhadis
Copy link
Collaborator

@Alhadis Alhadis commented Mar 1, 2018

This PR adds a script to sort the contents of .gitmodules each time a new grammar is added.

Description

We see this all the time:

This happens because newly-registered submodules are always added at the very bottom of the .gitmodules file, meaning that merging one PR usually blocks the next one until the submitter has resolved conflicts manually. This slows everybody down: a user has to fix the conflicts themselves, then @lildude needs to approve the changes later.

We can remedy this simply by forcing the submodule list to be alphabetised (case-sensitively). A test has been added to ensure that manually-added modules still get ordered correctly.

As of this writing, there are currently nine PRs sitting open which have conflicting module-lists: #4030, #4005, #3983, #3869, #3802, #3772, #3764, #3689, #3720.

I haven't even bothered to count how many that are closed which have had similar conflicts. 😉

Checklist:

  • I am adding new or changing current functionality
    • I have added or updated the tests for the new or changed functionality.

@pchaigno
Copy link
Contributor

pchaigno commented Mar 1, 2018

I haven't even bothered to count how many that are closed which have had similar conflicts. 😉

Who, you scared me. For a second, I read I have even 😲

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.

Definitely a good idea!

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.

I was thinking about doing this the other day after seeing another conflict. ✨ 🙇

@Alhadis Alhadis merged commit 1b3cdda into master Mar 2, 2018
@Alhadis Alhadis deleted the sort-modules branch March 2, 2018 09:33
@Alhadis
Copy link
Collaborator Author

Alhadis commented Mar 2, 2018

BTW, I ran into a few issues when testing the updated add-grammar script:

  1. Docker wasn't installed, so I had to sudo apt-get install docker

  2. Docker still wasn't installed, so I had to double-check what the hell the docker package was, and found out the correct name is actually docker-ce (??!)

  3. The correct Docker was installed, but it complained about inadequate permissions:

Warning: failed to get default registry endpoint from daemon (Got permission denied while trying to connect to the Docker daemon socket at unix:///var/run/docker.sock: Get http://%2Fvar%2Frun%2Fdocker.sock/v1.35/info: dial unix /var/run/docker.sock: connect: permission denied). Using system default: https://index.docker.io/v1/
Got permission denied while trying to connect to the Docker daemon socket at unix:///var/run/docker.sock: Post http://%2Fvar%2Frun%2Fdocker.sock/v1.35/images/create?fromImage=linguist%2Fgrammar-compiler&tag=latest: dial unix /var/run/docker.sock: connect: permission denied

This makes for some pretty unfriendly UX; should the scripts be updated to deal with these hurdles? Even bailing early in add-grammar would be preferable to bailing halfway, because each of the above three errors left the repository with half-registered grammar modules... 😞

@lildude
Copy link
Member

lildude commented Mar 2, 2018

This makes for some pretty unfriendly UX; should the scripts be updated to deal with these hurdles? Even bailing early in add-grammar would be preferable to bailing halfway, because each of the above three errors left the repository with half-registered grammar modules... 😞

Yeah, I thought this may have been a problem too. Wanna open an issue so we can track and not forget this?

@Alhadis
Copy link
Collaborator Author

Alhadis commented Mar 2, 2018

Done. 😉 See #4056.

@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.

3 participants