Page MenuHomePhabricator

Hook automatically fixing js formatting issues into git precommit
Open, LowPublic

Description

Motivation
Utilize eslint/stylelint/etc on pre-commit git hook so that I, as a developer, get all linting errors (possibly most or some of them fixed automatically) before I submit my changes to gerrit just to learn after 20 minutes (average time for our build) to learn about a missing space.

Suggested solution
Once T224398: Configure 'grunt fix' task for eslint on Wikibase.git is done, we can think of hooking one of the following combinations:

  1. have npm fix run alone as a pre-commit hook
  2. have npm test + fix run automatically
  3. only run npm test as part of the hook, developer runs grunt fix when needed

(Feedback is to be collected from other wikidata developers as to which is the best option)

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 23 2019, 9:32 AM
alaa_wmde added a subscriber: Addshore.

@Addshore Moving this to campsite candidate as technically speaking it shouldn't be that complicated nor that it will take long dev work.

alaa_wmde updated the task description. (Show Details)Jul 24 2019, 9:17 PM

@Addshore now I'm not so sure if it is that ready to be there, so I moved it back and updated the description.

noarave updated the task description. (Show Details)Aug 12 2019, 11:35 AM

thanks @noarave for preparing the task description.

Restricted Application added a project: Wikidata. · View Herald TranscriptAug 12 2019, 11:44 AM

Waiting on feedback from campers

Tarrow added a subscriber: Tarrow.Aug 12 2019, 12:03 PM

This sounds like a good plan. To me we should be also running the php linting/fixing at the same time in this case though.

In EntitySchema (package.json) we’ve been very happy with husky and lint-staged – I think @Michael is the expert there. (Unfortunately, PHPCS/PHPCBF’s exit codes don’t play nicely with it, but for the JS part it’s really wonderful.)

Yes, we had a good experience with this as EntitySchema. Do note, that in these pre-commit hooks, we did not use grunt, but the linters directly. The reason is that you cannot give files as CLI arguments to grunt and lint-staged is designed to only run the linting on the staged files.
This is basically option 1 of what you mentioned with an added git add where possible.

I found that approach helpful and would recommend it, even so it is not without edge-cases that need to be handled.

@Tarrow @Lucas_Werkmeister_WMDE @Michael Thank you for your input. I think we'll keep this task in the JS scope for now and go with something similar to what we did in EntitySchema. A separate task will be opened to explore similar options for PHP linting.

Addshore triaged this task as Low priority.Tue, Nov 5, 2:01 PM