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

Restore eslint rule for no-unused-vars #2078

Merged
merged 5 commits into from
Jun 30, 2021
Merged

Restore eslint rule for no-unused-vars #2078

merged 5 commits into from
Jun 30, 2021

Conversation

cmdcolin
Copy link
Collaborator

The eslint rule for unused vars got disabled at some point. This re-enables it and fixes some of the related errors

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Jun 25, 2021
@codecov
Copy link

codecov bot commented Jun 25, 2021

Codecov Report

Merging #2078 (254cc0c) into main (72d10c0) will increase coverage by 0.02%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2078      +/-   ##
==========================================
+ Coverage   61.38%   61.40%   +0.02%     
==========================================
  Files         479      479              
  Lines       22912    22909       -3     
  Branches     5250     5365     +115     
==========================================
+ Hits        14064    14068       +4     
+ Misses       8573     8559      -14     
- Partials      275      282       +7     
Impacted Files Coverage Δ
packages/core/TextSearch/TextSearchManager.ts 94.54% <ø> (ø)
packages/core/assemblyManager/assembly.ts 86.66% <ø> (ø)
packages/core/rpc/RpcManager.ts 86.66% <ø> (ø)
packages/core/ui/FileSelector.tsx 35.55% <ø> (ø)
packages/core/util/types/index.ts 62.96% <ø> (ø)
...ments/src/LinearSNPCoverageDisplay/models/model.ts 65.95% <ø> (ø)
...ginStoreWidget/components/InstalledPluginsList.tsx 84.61% <ø> (ø)
plugins/dotplot-view/src/PAFAdapter/PAFAdapter.ts 92.85% <ø> (ø)
.../legacy-jbrowse/src/JBrowse1Connection/jb1ToJb2.ts 0.00% <ø> (ø)
plugins/linear-comparative-view/src/index.tsx 27.36% <0.00%> (+0.14%) ⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72d10c0...254cc0c. Read the comment docs.

@cmdcolin cmdcolin added internal and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Jun 25, 2021
@cmdcolin cmdcolin force-pushed the add_no_unused_vars branch from b1df5d5 to abe05fb Compare June 28, 2021 13:49
@cmdcolin cmdcolin force-pushed the add_no_unused_vars branch from abe05fb to 3ad872a Compare June 28, 2021 15:15
@garrettjstevens
Copy link
Collaborator

I noticed this the other day, too, and I didn't have time to complete a fix, but I did find a configuration for this eslint rule that I thought might be useful. It's:

    "@typescript-eslint/no-unused-vars": [
      "warn",
      {
        "ignoreRestSiblings": true,
        "argsIgnorePattern": "^_",
        "varsIgnorePattern": "^_"
      }
    ],

Basically it allows unused vars/args if they have a "_" prefix, which can be useful when you want to indicate a name for what is being ignored. It also doesn't warn on unused rest siblings, which can be useful for "ignoring" parts of an object. See these changes in this branch: https://github.com/GMOD/jbrowse-components/tree/add_no_unused_vars_gs

@cmdcolin
Copy link
Collaborator Author

Interesting. Do you have any ideas for this issue? We could switch to using strict "import type" instead of plain import I think to fix this, but that would be a lot of lines of code changed https://github.com/GMOD/jbrowse-components/pull/2078/checks?check_run_id=2933281474#step:5:1160

Alternatively, we could...maybe consider using the normal no-unused-vars instead of @typescript-eslint/no-unused-vars

@cmdcolin
Copy link
Collaborator Author

Feel free to merge that branch into here if interested too

@garrettjstevens
Copy link
Collaborator

It's really weird that the lint output from the build doesn't match that of just running eslint, but I am able to reproduce it locally. I'm not sure import type will help, though, since some of the errors I get locally are for lines that already use import type. I have maybe one idea to check in react-scripts, I'll let you know if I find anything.

@cmdcolin cmdcolin marked this pull request as draft June 29, 2021 12:11
@garrettjstevens
Copy link
Collaborator

I'm not sure why the linting that happens as part of the webpack build isn't working correctly, but there is actually an option to disable that step of the build. I don't think we gain anything by linting during the build, since we lint everything separately anyway, so I've disabled it in my branch and everything works. Merging that branch in here now.

@cmdcolin cmdcolin marked this pull request as ready for review June 30, 2021 22:32
@cmdcolin
Copy link
Collaborator Author

thanks so much @garrettjstevens for helping to debug the issues with this PR.. expert level eslint engineering!

@cmdcolin cmdcolin merged commit ce7dc89 into main Jun 30, 2021
@cmdcolin cmdcolin deleted the add_no_unused_vars branch June 30, 2021 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants