Skip to content

(ci): add a lint job so PRs will require passing lint #378

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
Merged
merged 1 commit into from
Dec 18, 2019
Merged

(ci): add a lint job so PRs will require passing lint #378

merged 1 commit into from
Dec 18, 2019

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Dec 18, 2019 •
edited
Loading

  • a few recent PRs got in without passing lint, this breaks all future
    PRs/branches/commits because pre-commit will fail when it runs lint

Turns out lint was never running in CI! Which explains why #370 and #366 passed (and my confusion in #373 )
This should make formatting fixes like #372 and #377 unnecessary in the future if only PRs that pass lint are merged.

I limited this to run on Node v10 and ubuntu-latest -- not sure if a matrix is necessary for self-linting. The existing test suite already has tests for tsdx lint and that runs over the matrix ofc.

@agilgur5
(ci): add a lint job so PRs will require passing lint
06dc811 
- a few recent PRs got in without passing lint, this breaks all future
  PRs/branches/commits because pre-commit will fail when it runs lint
@agilgur5
Copy link
Collaborator Author

agilgur5 commented Dec 18, 2019 •
edited
Loading

Ok, this is a bit weird that it passes lint prior to #377 being merged...

The error I was getting was:

yarn run v1.19.1
$ yarn build && yarn lint:post-build
$ tsc -p tsconfig.json
$ node dist/index.js lint src test --ignore-pattern 'test/tests/lint'
=============

WARNING: You are currently running a version of TypeScript which is not officially supported by typescript-estree.

You may find that it works just fine, or you may not.

SUPPORTED TYPESCRIPT VERSIONS: >=3.2.1 <3.7.0

YOUR TYPESCRIPT VERSION: 3.7.3

Please only submit bug reports when using the officially supported version.

=============

~/Desktop/GitHub/tsdx/test/fixtures/build-default/src/syntax/nullish-coalescing.ts
  5:24  error  Parsing error: Expression expected  prettier/prettier

~/Desktop/GitHub/tsdx/test/fixtures/build-default/src/syntax/optional-chaining.ts
  3:52  error  Parsing error: Expression expected  prettier/prettier

✖ 2 problems (2 errors, 0 warnings)

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

But then the prettier errors stopped happening once I added the fix, including on local, despite that not being part of this branch... 🤔

@swyxio
Copy link
Collaborator

swyxio commented Dec 18, 2019 •
edited
Loading

great calls in every respect.

@swyxio swyxio merged commit 930feb9 into jaredpalmer:master Dec 18, 2019
@agilgur5
Copy link
Collaborator Author

Ah, I see, #370 added a commit later which bumped prettier which bumped its dependency on @typescript-eslint/typescript-estree, so it then started to pass lint when deps were upgraded.

#377 was still needed to get rid of the big warning and to support optional chaining / nullish coalescing in eslint proper (not just in prettier), but not to fix the specific prettier errors I wrote of above.

sebald added a commit to sebald/tsdx that referenced this pull request Dec 19, 2019
@sebald
Merge branch 'master' into transpile-only
118dcb4 
* master: (26 commits)
  (deps/lint): upgrade @typescript-eslint to support ?. and ?? (jaredpalmer#377)
  (ci): add a lint job so PRs will require passing lint (jaredpalmer#378)
  (clean): remove .rts_cache_* from storybook gitignore (jaredpalmer#375)
  Add optional chaining and nullish coalescing operators support (jaredpalmer#370)
  Added Storybook template (jaredpalmer#318)
  (fix/ci): GitHub Actions should run on PRs as well
  (fix/format): formatting of jaredpalmer#366 didn't pass lint
  Add prepare script to generated project (jaredpalmer#334)
  default jest to watch mode when not in CI (jaredpalmer#366)
  (fix): respect tsconfig esModuleInterop flag (jaredpalmer#327)
  fix: minor typo
  update rollup deps and plugins
  update to ts 3.7
  Remove unnecessary yarn install command in GH action
  update README.md
  update README.md
  Use node_modules/.cache/... as cacheRoot (jaredpalmer#329)
  fix(lint): Only default to src test if they exist (jaredpalmer#344)
  Fix error when providing babel/preset-env without options (jaredpalmer#350)
  Replaced some sync methods for their async version
  ...
@agilgur5 agilgur5 mentioned this pull request Jan 31, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@ambroseus ambroseus ambroseus approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@agilgur5 @swyxio @ambroseus