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(lint-staged): markdownlint and prettier can race on md files #28306

Merged
merged 4 commits into from
Aug 2, 2023

Conversation

LeoMcA
Copy link
Member

@LeoMcA LeoMcA commented Jul 31, 2023

Description

Prevent possible race condition between prettier and markdownlint in lint-staged, changed to use the order defined already in the package.json fix:md and lint:md commands.

I don't believe the other rules will race, since:

  • yarn test:front-matter-linter doesn't appear to change the files, just runs jest on them
  • prettier wouldn't run anyway on *.{svg,png,jpeg,jpg,gif}

@OnkarRuikar I don't seem to be able to assign you as a reviewer, but it would be great if you can take a look :)

Additional details

https://github.com/okonet/lint-staged#task-concurrency

Related issues and pull requests

Relates to #27936

followed the markdownlint/prettier ordering defined in the package.json
fix:md and lint:md commands

see: https://github.com/okonet/lint-staged#task-concurrency

follow-up to: mdn#27936
@LeoMcA LeoMcA requested a review from queengooborg July 31, 2023 16:10
@LeoMcA LeoMcA requested a review from a team as a code owner July 31, 2023 16:10
@github-actions github-actions bot added the system [PR only] Infrastructure and configuration for the project label Jul 31, 2023
@LeoMcA LeoMcA removed the request for review from a team July 31, 2023 16:11
Copy link
Collaborator

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

This seems like a good solution to me, LGTM!

@queengooborg
Copy link
Collaborator

Argh, but I can't merge this because the two required tests aren't running, since modified files don't match their scope.

.lintstagedrc.json Outdated Show resolved Hide resolved
@OnkarRuikar
Copy link
Contributor

OnkarRuikar commented Aug 1, 2023

Argh, but I can't merge this because the two required tests aren't running, since modified files don't match their scope.

Anyone, please review and merge this solution fast.

bsmth
bsmth previously requested changes Aug 1, 2023
Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

There's some changes outstanding requested by @OnkarRuikar in https://github.com/mdn/content/pull/28306/files#r1280214197

Co-authored-by: Onkar Ruikar <87750369+OnkarRuikar@users.noreply.github.com>
@bsmth bsmth dismissed their stale review August 1, 2023 10:28

Requested changes applied

@LeoMcA LeoMcA requested a review from mdn-bot as a code owner August 1, 2023 10:34
@LeoMcA LeoMcA merged commit 3a83d2b into mdn:main Aug 2, 2023
13 of 14 checks passed
@LeoMcA LeoMcA deleted the fix-lint-staged-race branch August 2, 2023 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
system [PR only] Infrastructure and configuration for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants