Skip to content
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.

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

chore: fix GH actions lint problem matchers issue in PR #4471

Merged
merged 1 commit into from
Mar 19, 2021

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Mar 19, 2021

Motivation

With #4447 we now lint on Github Actions and also use setup-node actions, enabling GH actions "problem matchers".

The problem is that now each PR has a list of reported lint errors in the files tab, like here: https://github.com/facebook/docusaurus/pull/4468/files

This is distracting PR reviews, so I'm using eslint --quiet option to ensure we don't care about warnings in CI and they don't end up in the PR.

cc @SamChou19815 , any alternative to suggest?

image

@slorber slorber added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Mar 19, 2021
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 19, 2021
@netlify
Copy link

netlify bot commented Mar 19, 2021

[V1] Deploy preview failure

Built with commit 7886815

https://app.netlify.com/sites/docusaurus-1/deploys/6054dd0b745bda00072744b6

@netlify
Copy link

netlify bot commented Mar 19, 2021

Deploy preview for docusaurus-2 ready!

Built with commit 7886815

https://deploy-preview-4471--docusaurus-2.netlify.app

@github-actions
Copy link

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 84
🟢 Accessibility 96
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-4471--docusaurus-2.netlify.app/

@SamChou19815
Copy link
Contributor

I think the point of the problem matcher is that we can see the lint warnings more easily, but I agree that the warnings on unchanged files can be annoying. Maybe we can try to lint only changed files on CI?

e.g. https://gist.github.com/seeliang/0f0de424d1cdc4541c338f4ee93b7e6a

@github-actions
Copy link

Size Change: 0 B

Total Size: 575 kB

ℹ️ View Unchanged
Filename Size Change
website/build/assets/css/styles.********.css 87.2 kB 0 B
website/build/assets/js/main.********.js 401 kB 0 B
website/build/blog/2017/12/14/introducing-docusaurus/index.html 60.8 kB 0 B
website/build/docs/introduction/index.html 235 B 0 B
website/build/index.html 25.7 kB 0 B

compressed-size-action

@slorber
Copy link
Collaborator Author

slorber commented Mar 19, 2021

Thanks, that's an idea but I think it's still useful to run on everything, at least on master.

Will merge this for now to reduce the noise, but that could be nice to have relevant warnings show in the PRs indeed :)

@slorber slorber merged commit acc9246 into master Mar 19, 2021
@armano2
Copy link
Contributor

armano2 commented Mar 20, 2021

maybe we should just fix those warnings instead of hiding them?

@slorber
Copy link
Collaborator Author

slorber commented Mar 22, 2021

maybe we should just fix those warnings instead of hiding them?

make sense, and we could re-enable the non-quiet CI linter.

There are 55 lint warnings left, do you want to fix those?

@slorber
Copy link
Collaborator Author

slorber commented Mar 22, 2021

maybe sync with @SamChou19815 as he did some cleanup recently and may already working on the last warnings?

@SamChou19815
Copy link
Contributor

@armano2 I'm not in the middle of fixing them right now. I'm currently trying to integrate esbuild into docusaurus.

@slorber slorber deleted the slorber/fix-eslint-problem-matchers branch August 17, 2021 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants