Skip to content

Conversation

KKonstantinov
Copy link
Contributor

This is a follow-up to #976 and it has a goal of improving DX experience, and reducing contributor friction by ensuring linting, formatting and tests are ran before commits.

It introduces the following:

  • Pre-commit: Lint Fix, Code Format, Build & Test (all parallel to speed up execution). Lint Fix, Code Format & Test running on staged files only and not on the full repo
  • Post-checkout & Post-merge: npm install (Just a convenience/quality of life addition)

Motivation and Context

PRs could get slowed down by failing when there is an accidental omission by the contributor to run lint/test/code format before committing.

Introducing this change will reduce contributor friction.

How Has This Been Tested?

Tested pre-commit with staged files, pre-commit with all files (npx lefthook run pre-commit --all-files)

Breaking Changes

No breaking changes, a pure DX change.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

@KKonstantinov KKonstantinov requested review from a team and ihrpr October 3, 2025 11:05
@chipgpt
Copy link

chipgpt commented Oct 3, 2025

What happens if the test(s) fail? I once had a colleague tell me they don't like putting things like tests in a pre-commit script because you should never stop a developer from committing their code. To be fair, this was in the context of employees working on a commercial software stack, not an open source SDK. Still, it has stuck with me and I think there is something to it.

Example might be if a developer is going out of town for a week and wants to commit their work in progress on a feature. If it is failing a test and can't be committed, then no one else can pick it up while they are out.

@KKonstantinov
Copy link
Contributor Author

What happens if the test(s) fail? I once had a colleague tell me they don't like putting things like tests in a pre-commit script because you should never stop a developer from committing their code. To be fair, this was in the context of employees working on a commercial software stack, not an open source SDK. Still, it has stuck with me and I think there is something to it.

Example might be if a developer is going out of town for a week and wants to commit their work in progress on a feature. If it is failing a test and can't be committed, then no one else can pick it up while they are out.

They can still do git commit --no-verify and skip the check on-demand - should not be a reason to skip a full step in the checks.

@domdomegg
Copy link
Member

Appreciate the PR! Maybe we should discuss this in an in issue/discussion to figure out what we want to agree on?

fwiw my view is that if you want to run things locally you always can, and if not CI will be there to catch this. I've personally found pre-commit/other hooks burn more of my time than they save, or are frustrating when trying to commit a WIP or draft piece of work (appreciate can use --no-verify, but then it's extra stuff to do and doesn't work with GUI git tools, AI agents get confused etc.). Although will flag that I'm not a TypeScript SDK maintainer, and of course there are reasonable differences of opinion here.

@pcarleton
Copy link
Member

could we do pre-push by default, and make pre-commit optional?

@KKonstantinov KKonstantinov requested a review from a team as a code owner October 8, 2025 05:53
@KKonstantinov
Copy link
Contributor Author

could we do pre-push by default, and make pre-commit optional?

This is a great idea and alleviates a lot of the concerns.

Done and pushed.

cc @pcarleton

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.

4 participants