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 GitHub 'Unchanged files with check annotations' reports in PR #26702

Merged
merged 2 commits into from Jan 28, 2024

Conversation

mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Jan 20, 2024

SUMMARY

Addressing the GitHub-reported lint warnings that show up in every PR. It's unclear to me why those warnings are surfaced by not others, but let's silence those clean up future PR from this. Note that there are 255 other warnings that are not surfaced in PRs, wondering if they do some sort of top N (?). We'll know soon enough.

Screenshot 2024-01-20 at 1 40 52 PM

Related issue filed with GitHub about this annoying beta feature -> actions/toolkit#457

@@ -37,7 +37,7 @@
"src/setup/*"
],
"scripts": {
"_lint": "eslint --ignore-path=.eslintignore --ext .js,.jsx,.ts,tsx .",
"_lint": "eslint --ignore-path=.eslintignore --ext .js,.jsx,.ts,tsx",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing the . at the end so this private base command can be reused more generically

@mistercrunch mistercrunch force-pushed the fix_lint branch 2 times, most recently from b1c8f63 to e83cffa Compare January 20, 2024 22:32
Copy link

codecov bot commented Jan 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0f59079) 67.16% compared to head (5b2bab7) 67.16%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #26702      +/-   ##
==========================================
- Coverage   67.16%   67.16%   -0.01%     
==========================================
  Files        1894     1894              
  Lines       74175    74175              
  Branches     8243     8243              
==========================================
- Hits        49821    49818       -3     
- Misses      22285    22288       +3     
  Partials     2069     2069              
Flag Coverage Δ
javascript 56.86% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mistercrunch
Copy link
Member Author

mistercrunch commented Jan 22, 2024

Oh it looks like the "Unchanged files with check annotations" is reporting some top N of eslint warnings, not this particular subset I tackled in this PR, which there are 268 at this time.

@mistercrunch
Copy link
Member Author

I'm going to lookup a way to shut off the stupid unrelated lint warnings as I really don't think they help. We know we have lint warning, and they really don't help in the context of an unrelated PR.

@mistercrunch
Copy link
Member Author

Commented here, going to look and see if there's a way to run eslint with no-warnings in CI

@mistercrunch
Copy link
Member Author

I think prior PR fixed #26704 the reporting of these, with --quiet now properly taking effect as it's passed to eslint, not tsc like it was before. Let's enjoy some squeaky clean PRs with no unrelated smell warnings.

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meant to review this sooner... I thought some of these estlint-disable bits might be in actual UI files files. I don't think we should be sweeping any under the rug in those cases but I don't think it's a problem in these test files.

@rusackas rusackas merged commit dfc614b into apache:master Jan 28, 2024
27 checks passed
eschutho pushed a commit to preset-io/superset that referenced this pull request Jan 31, 2024
@mistercrunch
Copy link
Member Author

Yeah in the end what fixed the annoying messages in PRs was to pass a --quiet flag to eslint in the context of CI. The warnings are still very much there, just not in CI / PRs.

sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.0.0 labels Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 4.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants