Skip to content

ci: resolve husky pre-push and pre-commit errors#10365

Merged
benelan merged 1 commit intodevfrom
benelan/fix-pre-commit-error
Sep 23, 2024
Merged

ci: resolve husky pre-push and pre-commit errors#10365
benelan merged 1 commit intodevfrom
benelan/fix-pre-commit-error

Conversation

@benelan
Copy link
Copy Markdown
Contributor

@benelan benelan commented Sep 20, 2024 •
edited
Loading

Summary

Resolve a few errors in our pre-commit and pre-push hooks:

  1. The support/updateStylelintCustomSassFunctions.ts script was trying to open scss files in the root directory.

    Error reading file: /home/jamin/dev/work/calcite-design-system/dev/ComponentWithSass.scss
Error: ENOENT: no such file or directory, open '/home/jamin/dev/work/calcite-design-system/dev/ComponentWithSass.scss'
    at Object.readFileSync (node:fs:448:20)
    at <anonymous> (/home/jamin/dev/work/calcite-design-system/dev/support/updateStylelintCustomSassFunctions.ts:37:24)
    at Array.forEach (<anonymous>)
    at sassFiles (/home/jamin/dev/work/calcite-design-system/dev/support/updateStylelintCustomSassFunctions.ts:35:11)
    at Object.<anonymous> (/home/jamin/dev/work/calcite-design-system/dev/support/updateStylelintCustomSassFunctions.ts:63:1)
    at Module._compile (node:internal/modules/cjs/loader:1358:14)
    at Object.transformer (/home/jamin/dev/work/calcite-design-system/dev/node_modules/tsx/dist/register-C1urN2EO.cjs:2:1122)
    at Module.load (node:internal/modules/cjs/loader:1208:32)
    at Module._load (node:internal/modules/cjs/loader:1024:12)
    at cjsLoader (node:internal/modules/esm/translators:348:17) {
errno: -2,
code: 'ENOENT',
syscall: 'open',
path: '/home/jamin/dev/work/calcite-design-system/dev/ComponentWithSass.scss'
}

  2. The stylelint config file extension was incorrect.

    Stylelint configuration updated successfully
fatal: pathspec 'packages/calcite-components/.stylelintrc.json' did not match any files

  3. The pre-push hook was only protecting main when it should protect rc and dev too.

  4. Husky requires scripts to be POSIX compliant, which means we can't use bash features.

    • Replace the read prompt's -p and -n flags with a printf and case statement

    • Replace double bracket conditionals used for pattern matching with grep -qE in the pre-commit hook

      .husky/pre-commit: 40: [[: not found
.husky/pre-commit: 16: [[: not found

@benelan benelan requested a review from jcfranco as a code owner September 20, 2024 21:32
@github-actions github-actions Bot added the chore Issues with changes that don't modify src or test files. label Sep 20, 2024
@benelan benelan added the skip visual snapshots Pull requests that do not need visual regression testing. label Sep 20, 2024
benelan
benelan commented Sep 20, 2024
View reviewed changes
Comment thread support/updateStylelintCustomSassFunctions.ts
@@ -15,7 +15,7 @@ function collectSassFiles(dir: string): string[] {

try {
fs.readdirSync(dir, { recursive: true, withFileTypes: true }).forEach(dirent => {
Copy link
Copy Markdown
Contributor Author

@benelan benelan Sep 20, 2024

Choose a reason for hiding this comment

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

Do we need to recursively check files starting in the root directory or can we be more specific?

The only relevant directories are packages/calcite-components/src and packages/calcite-design-system/dist/scss right?

Copy link
Copy Markdown
Member

@jcfranco jcfranco Sep 23, 2024

Choose a reason for hiding this comment

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

Is the suggestion to narrow the search scope mainly for performance reasons? I’d like to avoid an allowlist, so we don’t need to (remember to) update this script if our Sass file directories change.

jcfranco
jcfranco approved these changes Sep 23, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Thanks for fixing, @benelan!

Comment thread support/updateStylelintCustomSassFunctions.ts
@@ -15,7 +15,7 @@ function collectSassFiles(dir: string): string[] {

try {
fs.readdirSync(dir, { recursive: true, withFileTypes: true }).forEach(dirent => {
Copy link
Copy Markdown
Member

@jcfranco jcfranco Sep 23, 2024

Choose a reason for hiding this comment

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

Is the suggestion to narrow the search scope mainly for performance reasons? I’d like to avoid an allowlist, so we don’t need to (remember to) update this script if our Sass file directories change.

Comment thread .husky/pre-commit
fi
done

if [ -n "$(git diff --name-only -- "packages/calcite-components/.stylelintrc.json")" ]; then
Copy link
Copy Markdown
Member

@jcfranco jcfranco Sep 23, 2024

Choose a reason for hiding this comment

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

🤦‍♂️ Thanks for fixing this one.

@benelan benelan merged commit 3c458b5 into dev Sep 23, 2024
@benelan benelan deleted the benelan/fix-pre-commit-error branch September 23, 2024 17:32
benelan added a commit that referenced this pull request Sep 30, 2024
@benelan
Merge remote-tracking branch 'origin/dev' into benelan/8359-track-mil…
22a09da 
…estone-estimates

* origin/dev: (29 commits)
  fix(input-time-zone): fix region mode labeling and value mapping (#10345)
  fix(dropdown): open dropdown on `ArrowDown` & `ArrowUp` keys (#10264)
  refactor(components): reduce post-migration TypeScript errors (#10356)
  refactor: do not use Host in functional components (#10352)
  refactor(tests): reduce TypeScript errors (#10344)
  feat(alert): add component tokens (#10218)
  fix(card): properly handle slotted elements (#10378)
  refactor(panel): remove duplicate tailwind class (#10379)
  feat(popover, action): add component tokens (#10253)
  chore(t9n): add translations (#10339)
  feat: add 3d building, 3d building parameter, divide, parcel, spaces, spaces parameter, square brackets x, n variable, zoning parameter (#10373)
  build: update browserslist db (#10370)
  ci: resolve husky pre-push and pre-commit errors (#10365)
  docs: update component READMEs (#10371)
  feat(text-area): add component tokens (#10343)
  fix(action): prefer `disabled` in favor of `aria-disabled` (#10367)
  docs(a11y): add reference to a11y guidance to issue template (#10372)
  chore(action): add new string for accessible indicator label (#10360)
  feat(chip): add component tokens (#10179)
  feat(avatar): add component tokens (#10280)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jcfranco jcfranco jcfranco approved these changes

Assignees

@benelan benelan

Labels

chore Issues with changes that don't modify src or test files. skip visual snapshots Pull requests that do not need visual regression testing.

Projects

None yet

Milestone

2024-09-30 - 2.13.0 Sep Main release

Development

Successfully merging this pull request may close these issues.

2 participants

@benelan @jcfranco