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

Do some just+pre-commit tweaking #2776

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Do some just+pre-commit tweaking #2776

wants to merge 1 commit into from

Conversation

redsun82
Copy link
Contributor

  • pre-commit: move the linting check ahead of the compiling one, as a typescript lint can change the compiled javascript, so you can end up in a situation where the pre-commit check fails twice in a row, while now one run should always work
  • just: add linting and make the default to run all

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

* pre-commit: move the linting check ahead of the compiling one, as a
  typescript lint can change the compilaed javascript, so you can end up
  in a situation where the pre-commit check fails twice in a row
* just: add linting and make the default to run all
@Copilot Copilot bot review requested due to automatic review settings February 20, 2025 11:02
@redsun82 redsun82 requested a review from a team as a code owner February 20, 2025 11:02

Choose a reason for hiding this comment

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

PR Overview

This PR refactors the pre-commit hook ordering to run TypeScript linting before compiling, helping to reduce redundant error reporting. Additionally, the README.md file has been entirely removed.

  • Reorders hooks in .pre-commit-config.yaml so that the lint-ts hook runs before compile-ts.
  • Removes the README.md file, which may impact project documentation.

Reviewed Changes

File Description
.pre-commit-config.yaml Added lint-ts hook before compile-ts to avoid double failure on lint.
README.md Removed README.md, potentially impacting existing documentation.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

README.md:1

  • The removal of the README.md file may result in the loss of important project documentation. If this change is intentional, please ensure that the documentation is provided elsewhere and all related references are updated.
Entire README content removed.

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

@redsun82 redsun82 enabled auto-merge February 20, 2025 16:39
Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

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

I think you've deleted the README for the repo on accident 😆 looks good to me otherwise though!

@@ -1,3 +1,10 @@
# Perform all working copy cleanup operations
all: lint sync
Copy link
Contributor

Choose a reason for hiding this comment

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

Bikeshed: does all usually imply that it includes all the recipes (in this case also build)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👋 sync includes both build and update-pr-checks, so transitively all includes indeed all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes I see! Missed that 🤦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants