Skip to content

Sk/ssf 132 verify prettier check#99

Open
swarkewalia wants to merge 33 commits intomainfrom
sk/SSF-132-verify-prettier-check
Open

Sk/ssf 132 verify prettier check#99
swarkewalia wants to merge 33 commits intomainfrom
sk/SSF-132-verify-prettier-check

Conversation

@swarkewalia
Copy link

ℹ️ Issue
Closes 132

📝 Description
Added prettier verification in our CI/CD pipeline so it runs on pushed commits and fails when there are formatting violations, and passes when they are fixed.

✔️ Verification
Tested by running yarn prettier:check and pushing a commit that I knew had formatting violations to make sure the workflow failed. Then I commited and pushed fixes by running yarn prettier:write and made sure the workflow passed since there were no violations anymore.

Copy link
Member

@maxn990 maxn990 left a comment

Choose a reason for hiding this comment

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

Nice Swar! Left a few comments, but overall looks awesome. Looks like there are some formatting issues too, so just make sure to run prettier :)

} as unknown as SelectQueryBuilder<Order>;
beforeEach(async () => {
// Run all migrations fresh for each test
await testDataSource.runMigrations();
Copy link
Member

Choose a reason for hiding this comment

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

Could we clear the data and keep the schema instead of running the migrations every time? This could be quite slow

Copy link
Member

Choose a reason for hiding this comment

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

Ok so Dalton told me why we couldn't do this. The migration that seeds the db has migrations that run on top of it. I think it should be fine since your migrations run really fast.

package.json Outdated
Comment on lines 17 to 18
"prettier:check": "prettier --check \"**/*.{ts,tsx,js,jsx,json,md}\"",
"prettier:write": "prettier --write \"**/*.{ts,tsx,js,jsx,json,md}\""
Copy link
Member

Choose a reason for hiding this comment

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

Adding a more specific path like "apps/" could help to exclude node_modules and dist, etc since we don't need to check those every time

@swarkewalia swarkewalia requested a review from maxn990 February 15, 2026 18:10
Copy link
Member

@maxn990 maxn990 left a comment

Choose a reason for hiding this comment

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

niceeee looks really good!! one super small comment i missed the first time then i think we'll be ready to go

Comment on lines 43 to 45
- run: yarn install --frozen-lockfile
- run: yarn list strip-ansi string-width string-length
- run: yarn test No newline at end of file
- run: npx jest No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

i think these things should be moved to jobs from services

@maxn990 maxn990 requested a review from Yurika-Kan February 16, 2026 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants