Switch ApacheConf to maintained upstream by nisbet-hubbard · Pull Request #7143 · github-linguist/linguist · GitHub
Skip to content

Switch ApacheConf to maintained upstream#7143

Merged
lildude merged 7 commits into
github-linguist:mainfrom
nisbet-hubbard:patch-1
Jan 14, 2025
Merged

Switch ApacheConf to maintained upstream#7143
lildude merged 7 commits into
github-linguist:mainfrom
nisbet-hubbard:patch-1

Conversation

@nisbet-hubbard

@nisbet-hubbard nisbet-hubbard commented Dec 1, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Discussed in #7135.

Checklist:

@nisbet-hubbard nisbet-hubbard requested a review from a team as a code owner December 1, 2024 05:27
lildude
lildude requested changes Dec 1, 2024
View reviewed changes

@lildude lildude left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use the documented method of changing grammars.

@nisbet-hubbard

nisbet-hubbard commented Dec 1, 2024

Copy link
Copy Markdown
Contributor Author

@nisbet-hubbard nisbet-hubbard reopened this Dec 1, 2024
lildude
lildude requested changes Dec 2, 2024
View reviewed changes

@lildude lildude left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The script should have also created a yaml file containing the grammar licence. We need that file too. If it wasn't created or you've cleaned up, running just this line should do the trick:

linguist/script/add-grammar

Line 135 in 5fad8d5

bundle exec licensed cache -c vendor/licenses/config.yml

@lildude

lildude commented Dec 2, 2024

Copy link
Copy Markdown
Member

Oooof. Looks like @michidk has renamed the typst grammar repo. I've opened #7145 to track this.

@lildude

lildude commented Dec 3, 2024

Copy link
Copy Markdown
Member

Oooof. Looks like @michidk has renamed the typst grammar repo. I've opened #7145 to track this.

Resolved now. The test failures are legit. Looks like you may not have used the script/add-grammar command as documented. Please use that script to replace the grammar.

@nisbet-hubbard

nisbet-hubbard commented Dec 6, 2024

Copy link
Copy Markdown
Contributor Author

Looks like you may not have used the script/add-grammar command as documented.

That's exactly what I did. What else is missing?

@lildude

lildude commented Dec 7, 2024

Copy link
Copy Markdown
Member

That's exactly what I did. What else is missing?

See the test failures. They indicate the grammar may not have been swapped using the script as it should have prevented all of those issues as it updates all the files reported in those errors.

@nisbet-hubbard

nisbet-hubbard commented Dec 8, 2024

Copy link
Copy Markdown
Contributor Author

So what happened appears to be that after running the script, three files got automatically staged whilst the licence and grammars.yml had to be manually staged.

These I’ve now committed.

lildude
lildude requested changes Dec 8, 2024
View reviewed changes

@lildude lildude left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Whoops. The grammars.yaml file has waaaaaaay too many changes.

@nisbet-hubbard

nisbet-hubbard commented Dec 8, 2024

Copy link
Copy Markdown
Contributor Author

Sorry, fixed that!

lildude
lildude requested changes Dec 8, 2024
View reviewed changes

@lildude lildude left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The vendor/README.md should have also been updated by the script.

@lildude

lildude commented Jan 14, 2025

Copy link
Copy Markdown
Member

I'm really not sure why you encountered problems as I've just reverted all your changes and replaced the grammar as documented and it's worked a treat. I've just pushed the changes to your fork.

lildude
lildude approved these changes Jan 14, 2025
View reviewed changes

@lildude lildude left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM now 😁

Important

The changes in this PR will not appear on GitHub until the next release has been made and deployed. See here for more details.

@lildude lildude added this pull request to the merge queue Jan 14, 2025
Merged via the queue into github-linguist:main with commit 1a30b0f Jan 14, 2025
@nisbet-hubbard

nisbet-hubbard commented Jan 18, 2025

Copy link
Copy Markdown
Contributor Author

@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jul 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lildude lildude lildude approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nisbet-hubbard @lildude