chore(deps): upgrade @dhis2/cli-style to latest alpha

[ghost]

This isn't really a review for the proposed changes in this PR, just used this to have some context for the cli-style changes on alpha. I've just noted down what stood out. I don't think that it's really necessary to deal with all the notes for the current release, we could deal with some of these issues later.

Todo

• Fix CI
• Add back all the old checks
• Try lslint

Short notes

• Should we disable the pre push jest test by default? I would prefer doing so until we have enabled this: https://jestjs.io/docs/next/cli#--lastcommit flag (or something similar, depending on when we want to run the tests). Currently it delays each of my commits by half a minute. That pushes me towards just skipping the tests with --no-verify, and I've heard that from others as well. I think we'd probably want an approach that doesn't push people towards ignoring it and making a habit out of that (ignoring).
• Terminal colors are missing for me from the report generated by d2-style check. Though I see they're working on CI, and they're working in some areas for me locally. See how here the d2-style check and javascript > eslint string are highlighted correctly, but not the warning string for example. I'm not using true color (terminal is true color capable though), just the 16 ANSI colors. Are we using the correct ANSI escape sequences everywhere?
• The debug info that's being logged is drowning out the linting messages on CI: https://github.com/dhis2/scheduler-app/runs/2381877024?check_suite_focus=true#step:6:121 (this kind of disincentivizes checking the linting errors/warnings, and pushes you towards just ignoring everything that's being logged).
• The pre commit linting takes quite long for me. It's runnning d2-style check so I'm assuming we don't just lint the staged files. For me the checks are a bit too slow for a regular commit. Similarly to the unit tests before push, I'd say that that encourages using --no-verify.
• I like that we're using .hooks and not .d2, that way we can make exceptions on a project to project basis if necessary (assuming that we want to keep everything in .d2 out of version control, if not then .d2 would work as well of course).
• I've tried ls-lint. I used d2-style add ls-lint and then d2-style check fs on the latest commit of this branch. Works well, though initially I was surprised that it didn't ignore .d2. I expected d2-style to ignore everything in my gitignore, just like prettier and eslint. But I see that ls-lint has its own config with an ignore section. Maybe we should add .d2 to the defaults there.
• One other thing I noticed with ls-lint is that it seems to consider multiple extensions a violation: 2021/04/20 16:15:27 ./src/hooks/jobs/use-submit-job.test.js failed for rules: kebabcase. I would have expected it to allow this to pass as the test.js extension is pretty common across our codebases and not really violating the no-caps rule. At least I assume that that is why it's failing. Also happens with d2 config for example: 2021/04/20 16:15:27 ./d2.config.js failed for rules: kebabcase

Hooks

The default hooks, installed with d2-style install, didn't work for me. This for example:

#!/bin/sh
. "$(dirname "$0")/_/husky.sh"

./bin/d2-style check commit "$1"

Exits with: /Users/ismay/Projects/github/dhis2/scheduler-app/.hooks/commit-msg: line 4: ./bin/d2-style: No such file or directory. pwd in that script yields: /Users/ismay/Projects/github/dhis2/scheduler-app so I guess that makes sense?

I changed the hooks in this commit (654996d), following these instructions: https://typicode.github.io/husky/#/?id=locally-installed-binaries (running them via the package manager basically), and that fixed it.

[ghost]

Uploaded here so I could link it above 😋

[varl]

./bin/d2-style check commit "$1"

This is actually specific for the dhis2/cli-style repo, because it needs to run itself. In an app, we should use yarn d2-style check --staged "$1" in the pre-commit hook.

[varl]

• The debug info that's being logged is drowning out the linting messages on CI: https://github.com/dhis2/scheduler-app/runs/2381877024?check_suite_focus=true#step:6:121 (this kind of disincentivizes checking the linting errors/warnings, and pushes you towards just ignoring everything that's being logged).

Debug messages in cli-style are heavily directed at being useful for debugging, so I'm a bit surprised to see the D2_VERBOSE env variable set to true in CI. Perhaps we need to differentiate between debugging and verbose logging..

[varl]

• The pre commit linting takes quite long for me. It's runnning d2-style check so I'm assuming we don't just lint the staged files. For me the checks are a bit too slow for a regular commit. Similarly to the unit tests before push, I'd say that that encourages using --no-verify.

The pre-commit hook should run yarn d2-style check --staged.

[varl]

• Should we disable the pre push jest test by default?

Sure, as long as we run the tests on CI I'm OK with that.

[varl]

• But I see that ls-lint has its own config with an ignore section. Maybe we should add .d2 to the defaults there.

I missed this detail. 👍

[varl]

• One other thing I noticed with ls-lint is that it seems to consider multiple extensions a violation: 2021/04/20 16:15:27 ./src/hooks/jobs/use-submit-job.test.js failed for rules: kebabcase.

Yes, this one is annoying. They call that "point.case" in ls-lint and we need to figure out how to handle it.

[varl]

• Terminal colors are missing for me from the report generated by d2-style check.

This is why.

[ghost]

This is actually specific for the dhis2/cli-style repo, because it needs to run itself. In an app, we should use yarn d2-style check --staged "$1" in the pre-commit hook.

Yeah exactly. So it seems to install the hooks that were meant for internal usage in the repo in this app when I ran d2-style install, and not the yarn prefixed ones. Is that a bug then?

Debug messages in cli-style are heavily directed at being useful for debugging, so I'm a bit surprised to see the D2_VERBOSE env variable set to true in CI. Perhaps we need to differentiate between debugging and verbose logging..

Ah so that's why. I took it from our shared workflows repo: https://github.com/dhis2/workflows/blob/master/ci/dhis2-verify-app.yml. So I'd say that we should probably set this to false by default then. What do you think?

The pre-commit hook should run yarn d2-style check --staged.

I agree. So it seems that the default hook it installed for me was without the --staged. Is that because of config in this repo, or is that something we need to address in cli-style?

Sure, as long as we run the tests on CI I'm OK with that.

That's the direction I'd go I think. Though I could also see us using onlyChanged in the pre commit hook.

Yes, this one is annoying. They call that "point.case" in ls-lint and we need to figure out how to handle it.

I see. Yeah tricky that there's a bit of overlap in our naming conventions.

Terminal colors are missing for me from the report generated by d2-style check..
This is why.

I see, thanks. Why do we disable colors there? Personally I find it quite useful as a quick way to scan for errors/warnings when I'm scrolling through a list of them.

[ghost]

Let me know by the way if it would help if I submit PRs for the changes I'm proposing.

[varl]

Let me know by the way if it would help if I submit PRs for the changes I'm proposing.

I mean, yes? 😆

[varl]

I see, thanks. Why do we disable colors there? Personally I find it quite useful as a quick way to scan for errors/warnings when I'm scrolling through a list of them.

Decisions made in another time/dimension intersection. We can remove this now that the philosophy has changed.

[ghost]

I mean, yes? 😆

Haha, yeah I guess I meant, do you agree, and shall I submit a PR. Don't know if you were already working on it. More in that manner. Didn't mean to ask: "Hey, is it helpful if I help?" 😋

[varl]

Yeah exactly. So it seems to install the hooks that were meant for internal usage in the repo in this app when I ran d2-style install, and not the yarn prefixed ones. Is that a bug then?

Yeah, that's a bug.

I agree. So it seems that the default hook it installed for me was without the --staged. Is that because of config in this repo, or is that something we need to address in cli-style?

I don't see the d2-style.config.js file in this repo, so I think it's something in cli-style.

[varl]

I mean, yes? laughing

Haha, yeah I guess I meant, do you agree, and shall I submit a PR. Don't know if you were already working on it. More in that manner. Didn't mean to ask: "Hey, is it helpful if I help?" yum

No, I hadn't caught these issues but now that you've pointed them out I can fix them and assign you to review.

[ghost]

I mean, yes? laughing

Haha, yeah I guess I meant, do you agree, and shall I submit a PR. Don't know if you were already working on it. More in that manner. Didn't mean to ask: "Hey, is it helpful if I help?" yum

No, I hadn't caught these issues but now that you've pointed them out I can fix them and assign you to review.

👍 Cool, sounds good!

[varl]

• eslint colors: fix: allow eslint to print colors cli-style#384
• wrong hooks installed by default: fix: wrong hooks where installed by default cli-style#385
• ignore .d2: fix: ignore .d2 directory cli-style#386

[varl]

Ah so that's why. I took it from our shared workflows repo: https://github.com/dhis2/workflows/blob/master/ci/dhis2-verify-app.yml. So I'd say that we should probably set this to false by default then. What do you think?

I am pretty sure we had that as true there while debugging, and just forgot to switch it to false, so I agree.

[ghost]

I'll close this PR as it's served its purpose as a testbed. I'll create follow up issues for anything that hasn't been addressed yet.