ci: continuing linting when one kind errors #4416

turadg · 2022-01-28T19:16:57Z

Description

It's helpful to developers that when CI fails you are informed what you need to do to make it pass. Presently when one kind of lint check fails, the logic fails fast and you get no information about subsequent ones. This was due to the steps in Github workflow and the && in the yarn lint scripts.

This makes the yarn lint command check all the defined lint:foo commands and continue when any error. It accomplishes this using https://github.com/mysticatea/npm-run-all .

It also:

update CI to use the one yarn lint so its steps don't fail fast
remove lint-check command since that's what lint does (checks)
removes tsc from lint-fix since it can't do any fixing.
remove the commands that used .eslintrc-jessie.js because that files doesn't exist (superceded by plugin:@jessie.js/recommended)

Security Considerations

Introduction of a new package. It's only a devDependency and is maintained by a well-regarded project lead who's a member of ESLint, Babel, and Vue.js.

Documentation Considerations

Notify developers to that yarn lint-check no longer works and to use yarn lint instead.

Testing Considerations

Locally tested that the different jobs run and failure of any is conveyed for the lint overall.

CI passes.

dckc · 2022-01-28T19:39:26Z

The current lint check in CI ... conflates type checking with linting

conflates? or usefully combines? they're both incomplete static checks on code, yes?

I suggest surveying more of the team, but to my mind, it's a feature, not a bug, that yarn lint does both.

If you want separate lint:eslint and lint:types ci jobs, very well. But I'm not convinced there's a good reason to change what yarn lint means.

mhofman · 2022-01-28T19:43:02Z

I'm not sure about this. I'm actually more of the opinion of use "lint" as an umbrella not restricted to "eslint", but also for types and even an explicit "prettier" check, so I actually prefer the conflation.

As to the && making these sequential, it sure would be nice to have all output available when multiple errors exist, but is it really necessary?

turadg · 2022-01-28T21:24:11Z

conflates? or usefully combines? they're both incomplete static checks on code, yes?

Well taken. I was thinking of "lint" as "eslint" but that's just one type of lint.

As to the && making these sequential, it sure would be nice to have all output available when multiple errors exist, but is it really necessary?

I think it's a good requirement that when CI fails you are informed what you need to do to make it pass. Presently when one kind of lint check fails, you get no information about ones later in the && expression.

So that means:

~~conflates type checking with linting~~
the && expression means if any typechecks fail, linting isn't run

Instead I'll solve the latter so that the new CI check isn't necessary. (Saves setup time too.)

This reverts commit 7fc4f39.

dckc · 2022-02-01T17:12:39Z

README.md

-`node_modules/@agoric/marshal` is a symlink to `packages/marshal`).
+`node_modules/@endo/marshal` is a symlink to `packages/marshal`).


I doubt the symlink thing applies across monorepos. So maybe change the example from ERTP->marshal to Zoe->ERTP.

Dunno how that change got in the PR. Gone now.

dckc

I'm a little hesitant to speak for the team on this... especially without an issue. But I guess I'm OK to give this my usual level of approval, which is: if it turns out to be broken, I agree to be responsible to fix it.

mhofman · 2022-02-01T21:14:29Z

package.json

@@ -81,10 +82,10 @@
    "link-cli": "yarn run create-agoric-cli",
    "create-agoric-cli": "node ./scripts/create-agoric-cli.cjs",
    "format": "yarn prettier --write 'packages/**/*.{js,jsx,ts,tsx}' 'golang/**/*.{js,jsx,ts,tsx}'",


Should we rename to lint-fix:format in the principle of consistency? And introduce a lint-fix that performs the above and a workspace lint-fix (for the eslint fix)

I've been burned by eslint --fix enough not to trust it so I'd personally shy away from using a catch-all yarn lint-fix but I wouldn't object to a PR making that change.

Agreed! But I believe it's used by some. I personally relies on my editor for formatting auto fixes, so I use neither. I'll let others chime in.

mhofman · 2022-02-01T21:18:06Z

packages/bundle-source/package.json

-    "lint-fix-jessie": "eslint -c '.eslintrc-jessie.js' --fix '**/*.js'",
-    "lint-check-jessie": "eslint -c '.eslintrc-jessie.js' '**/*.js'",


Where did these go?

I'll mention in the notes: there is no .eslintrc-jessie.js so these weren't even working. We have "plugin:@jessie.js/recommended" in eslint-config so it's handled by that.

Ping @michaelfig I assume?

yes, plugin:@jessie.js/recommended along with @jessie-check is the current mechanism.

turadg added 4 commits January 28, 2022 10:40

format: prettier .github yml files

a7cb80b

for code quality checks, skip matrix and use the active LTS

d5b3e6e

separate types-check from lint-check

7fc4f39

exclude types from lint-fix

51fbaa8

turadg requested review from mhofman and dckc January 28, 2022 19:16

turadg mentioned this pull request Jan 28, 2022

refactor: unify TypeScript project #4404

Closed

Revert "separate types-check from lint-check"

6a551b7

This reverts commit 7fc4f39.

turadg marked this pull request as draft January 28, 2022 21:44

turadg added 6 commits February 1, 2022 07:23

Merge branch 'master' into ta/separate-typecheck

64fa9aa

single 'yarn lint' for all linting

34cb1de

single 'yarn lint' for all linting

0c467ab

remove 'lint-check', redundant with 'lint'

58cbc94

include Jessie linting

e115c9d

remove broken lint..jessie commands

f7d52ce

dckc reviewed Feb 1, 2022

View reviewed changes

turadg added 3 commits February 1, 2022 11:40

back out CI matrix and formatting change

2eb5176

use run-s consistently

d08b57f

Merge branch 'master' into ta/separate-typecheck

435626a

turadg changed the title ~~ci: move type checking from lint-check to new types-check~~ ci: continuing linting when one kind errors Feb 1, 2022

turadg marked this pull request as ready for review February 1, 2022 20:11

turadg requested a review from dckc February 1, 2022 20:12

dckc approved these changes Feb 1, 2022

View reviewed changes

turadg added the automerge:squash Automatically squash merge label Feb 1, 2022

mhofman reviewed Feb 1, 2022

View reviewed changes

turadg added automerge:squash Automatically squash merge and removed automerge:squash Automatically squash merge labels Feb 1, 2022

fix typo

fdb90e1

mergify bot merged commit f0eba3c into master Feb 1, 2022

mergify bot deleted the ta/separate-typecheck branch February 1, 2022 22:07

turadg mentioned this pull request Feb 23, 2022

build: no-bail when testing/linting whole repo #4646

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: continuing linting when one kind errors #4416

ci: continuing linting when one kind errors #4416

turadg commented Jan 28, 2022 •

edited

Loading

dckc commented Jan 28, 2022

mhofman commented Jan 28, 2022

turadg commented Jan 28, 2022 •

edited

Loading

dckc Feb 1, 2022 •

edited

Loading

turadg Feb 1, 2022

dckc left a comment

mhofman Feb 1, 2022 •

edited

Loading

turadg Feb 1, 2022

mhofman Feb 1, 2022

mhofman Feb 1, 2022

turadg Feb 1, 2022 •

edited

Loading

mhofman Feb 1, 2022

dckc Feb 1, 2022

		`node_modules/@agoric/marshal` is a symlink to `packages/marshal`).
		`node_modules/@endo/marshal` is a symlink to `packages/marshal`).

		"lint-fix-jessie": "eslint -c '.eslintrc-jessie.js' --fix '*/.js'",
		"lint-check-jessie": "eslint -c '.eslintrc-jessie.js' '*/.js'",

ci: continuing linting when one kind errors #4416

ci: continuing linting when one kind errors #4416

Conversation

turadg commented Jan 28, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

dckc commented Jan 28, 2022

mhofman commented Jan 28, 2022

turadg commented Jan 28, 2022 • edited Loading

dckc Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

turadg Feb 1, 2022

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

mhofman Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

turadg Feb 1, 2022

Choose a reason for hiding this comment

mhofman Feb 1, 2022

Choose a reason for hiding this comment

mhofman Feb 1, 2022

Choose a reason for hiding this comment

turadg Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

mhofman Feb 1, 2022

Choose a reason for hiding this comment

dckc Feb 1, 2022

Choose a reason for hiding this comment

turadg commented Jan 28, 2022 •

edited

Loading

turadg commented Jan 28, 2022 •

edited

Loading

dckc Feb 1, 2022 •

edited

Loading

mhofman Feb 1, 2022 •

edited

Loading

turadg Feb 1, 2022 •

edited

Loading