Problem/Motivation

As seen within #3345607: Update to Package Manager 3.x, there is a much less brittle approach to running commit checks

Steps to reproduce

Proposed resolution

Remaining tasks

  • ✅ File an issue about this project
  • ☐ Manual Testing
  • ☐ Code Review
  • ☐ Accessibility Review
  • ☐ Automated tests needed/written?
CommentFileSizeAuthor
#4 commit-checks.patch11.73 KBtim.plunkett

Issue fork project_browser-3348781

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett opened merge request !364

chrisfromredfin made their first commit to this issue’s fork.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review
StatusFileSize
new commit-checks.patch11.73 KB
chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Needs review » Fixed

Woo! Linting & testing back up and working.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Fixed » Needs work
Related issues: +#3343430: Stop reusing core's commit-code-check.sh in favor of 4 simple commands

I introduced this alternative approach for Package Manager/Automatic Updates in #3343430: Stop reusing core's commit-code-check.sh in favor of 4 simple commands. But there's one important difference between the two projects: PM/AU does not contain any JS, but PB does!

So right now you lost all JS code quality checks 😅 You'll want to port those over.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Good point @Wim Leers!

But I think we only have one non-Svelte JS file, and we were already going out of our way to disable JS checks for those, since they contain HTML and CSS as well.

That's why our version committed above has an additional step to run our lint:svelte command

I'll leave this for @chrisfromredfin to ponder, but I think we're okay here

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

👍

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented

As I've thought about this, it feels the same as the yaml linter. We could do it now or do it later, but the consequence regardless is minimal. If we discover it late, it's maybe 30 minutes to fix both, because the surface area is so small. It might be nice to open a lower-priority ticket to add those two things.

My larger concern is that the javascript that IS inside of .svelte files is passing Drupal JS checks, and I'm hoping lint:svelte is able to check those things.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Needs work » Closed (outdated)

Documenting here that yes, lint:svelte does check for proper formatting inside of the .svelte files, so I do think we're good here.