FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Do some just+pre-commit tweaking by redsun82 · Pull Request #2776 · github/codeql-action · GitHub

/ codeql-action Public

Do some just+pre-commit tweaking - #2776

Merged
redsun82 merged 1 commit into
mainfrom
redsun82/just
Feb 27, 2025
ConversationCommits (1)ChecksFiles changed
Merged

Do some just+pre-commit tweaking#2776
redsun82 merged 1 commit into
mainfrom
redsun82/just

Conversation

redsun82 commented Feb 20, 2025

Copy link
Copy Markdown
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.

Copilot AI review requested due to automatic review settings February 20, 2025 11:02
redsun82 requested a review from a team as a code owner February 20, 2025 11:02
Copilot AI reviewed Feb 20, 2025
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 enabled auto-merge February 20, 2025 16:39
angelapwen previously requested changes Feb 20, 2025
View reviewed changes

angelapwen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

angelapwen reviewed Feb 20, 2025
View reviewed changes
Comment thread justfile
@@ -1,3 +1,10 @@
# Perform all working copy cleanup operations
all: lint sync

angelapwen Feb 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

redsun82 Feb 21, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

angelapwen Feb 21, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh yes I see! Missed that 🤦

redsun82 requested a review from angelapwen February 24, 2025 11:48

redsun82 commented Feb 26, 2025

Copy link
Copy Markdown
Contributor Author

@angelapwen mind having back a look?

NlightNFotis commented Feb 26, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Hi @redsun82 , thank you for your work on improving our just infrastructure in the action!

Quick question: the files changes shows the readme.md file deleted - is there a reason for this I'm missing out on?

redsun82 commented Feb 26, 2025

Copy link
Copy Markdown
Contributor Author

Hi @redsun82 , thank you for your work on improving our just infrastructure in the action!

Quick question: the files changes shows the readme.md file deleted - is there a reason for this I'm missing out on?

Ah, no idea how that could happen, good catch! I'll fix that tomorrow!

Do some just+pre-commit tweaking
9663263 
* 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

redsun82 commented Feb 27, 2025

Copy link
Copy Markdown
Contributor Author

Hi @redsun82 , thank you for your work on improving our just infrastructure in the action!
Quick question: the files changes shows the readme.md file deleted - is there a reason for this I'm missing out on?

Ah, no idea how that could happen, good catch! I'll fix that tomorrow!

Fixed that, thanks again for spotting that!

NlightNFotis approved these changes Feb 27, 2025
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, thanks again for the improvement.

redsun82 dismissed angelapwen’s stale review February 27, 2025 11:40

removing changes requested to unblock merging

redsun82 merged commit 8392354 into main Feb 27, 2025
redsun82 deleted the redsun82/just branch February 27, 2025 11:40
github-actions Bot mentioned this pull request Mar 7, 2025
Merged
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
NlightNFotis NlightNFotis approved these changes
angelapwen Awaiting requested review from angelapwen

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL