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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Sep 23, 2024
Merged

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

merged 1 commit into from
Sep 23, 2024

Conversation

benelan
Copy link
Member

@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
support/updateStylelintCustomSassFunctions.ts
@@ -15,7 +15,7 @@ function collectSassFiles(dir: string): string[] {

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

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
Member

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
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!

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

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

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.

.husky/pre-commit
fi
done

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

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
20 checks passed
@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