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

Rewrite script/add-grammar as a plain shell-script #5521

Merged
merged 7 commits into from
Sep 2, 2021

Conversation

Alhadis
Copy link
Collaborator

@Alhadis Alhadis commented Aug 15, 2021

Description

I recently suggested that script/add-grammar be rewritten as a plain shell-script, since 90% of it is nothing but executing external commands. Really the only thing we need Ruby for here is normalising user-provided grammar specifiers, a function that's now handled with a new script: normalise-url:

$ script/normalise-url Alhadis/language-etc bitbucket:bitlang/sublime_cobol
https://github.com/Alhadis/language-etc.git
https://bitbucket.org/bitlang/sublime_cobol.git

I've gone and hardened the normalisation logic to catch and correct malformed URLs (when ambiguity isn't a concern), something which the current script does only a minimal job of. For example:

$ script/normalise-url --protocol=https git@GITHuB.CoM://////Alhadis/language-etc///////
https://github.com/Alhadis/language-etc.git
$ script/normalise-url git+ssh://github.com////github/linguist.git
git://github.com/github/linguist.git

Checklist:

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

/cc @lildude

@Alhadis Alhadis requested a review from a team as a code owner August 15, 2021 14:06
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.

A quick first pass-through with my handy linters. Mostly nits.

script/add-grammar Show resolved Hide resolved
script/add-grammar Show resolved Hide resolved
script/add-grammar Show resolved Hide resolved
script/add-grammar Show resolved Hide resolved
script/add-grammar Show resolved Hide resolved
script/normalise-url Outdated Show resolved Hide resolved
script/normalise-url Outdated Show resolved Hide resolved
script/normalise-url Outdated Show resolved Hide resolved
script/normalise-url Outdated Show resolved Hide resolved
script/normalise-url Outdated Show resolved Hide resolved
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.

Two more non-blocking quick fixes to keep things consistent so feel free to merge the suggestions and merge.

Thanks for this rewrite 🙇

script/add-grammar Outdated Show resolved Hide resolved
script/add-grammar Outdated Show resolved Hide resolved
@Alhadis Alhadis merged commit 8b3c18c into master Sep 2, 2021
@Alhadis Alhadis deleted the rewrite-grammar-script branch September 2, 2021 11:14
@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.

2 participants