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

Fix lint warnings #195

Merged
merged 1 commit into from
Jan 9, 2025
Merged

Fix lint warnings #195

merged 1 commit into from
Jan 9, 2025

Conversation

AndrewTwydell
Copy link
Contributor

@AndrewTwydell AndrewTwydell commented Jan 6, 2025

What It Does

Partial fix for #146.
Resolves basic lint warnings in packages. Does NOT resolve complexity or any return type warnings - these will be in a follow-up PR as they require more refactoring.

Introduces a constants file for repeated, magic values throughout the VSCE package.

Review Checklist
I certify that I have:

@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2025

Codecov Report

Attention: Patch coverage is 4.74138% with 221 lines in your changes missing coverage. Please review.

Project coverage is 28.38%. Comparing base (106365f) to head (6d8b94d).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
packages/vsce/src/extension.ts 0.00% 31 Missing ⚠️
...ackages/vsce/src/commands/showAttributesCommand.ts 0.00% 25 Missing ⚠️
...ckages/vsce/src/commands/filterResourceCommands.ts 0.00% 15 Missing ⚠️
packages/vsce/src/trees/CICSTree.ts 7.14% 13 Missing ⚠️
...trees/CICSCombinedTrees/CICSCombinedLibraryTree.ts 0.00% 7 Missing ⚠️
...s/CICSCombinedTrees/CICSCombinedTransactionTree.ts 0.00% 7 Missing ⚠️
...ackages/vsce/src/commands/closeLocalFileCommand.ts 0.00% 6 Missing ⚠️
...ommands/disableCommands/disableLocalFileCommand.ts 0.00% 6 Missing ⚠️
...mands/disableCommands/disableTransactionCommand.ts 0.00% 6 Missing ⚠️
packages/vsce/src/commands/openLocalFileCommand.ts 0.00% 6 Missing ⚠️
... and 27 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #195      +/-   ##
==========================================
+ Coverage   28.20%   28.38%   +0.17%     
==========================================
  Files         147      148       +1     
  Lines        5113     5081      -32     
  Branches      889      892       +3     
==========================================
  Hits         1442     1442              
+ Misses       3671     3639      -32     

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

@AndrewTwydell AndrewTwydell force-pushed the lint-fix branch 4 times, most recently from 355dae3 to bda99b5 Compare January 7, 2025 11:11
@AndrewTwydell AndrewTwydell marked this pull request as ready for review January 7, 2025 11:14
token.onCancellationRequested(() => {
console.log("Cancelling the loading of resources");
});
token.onCancellationRequested(() => { });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have removed the unecessary console logs from these methods, but kept the empty method for if we want to add a cancellation action - am willing to debate whether this is a good idea or not

Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing a lot of the lint warnings. 🙏

We often don't worry too much about the cognitive complexity ones. And we understand that the @typescript-eslint/no-unsafe-return can turn into a more complicated fix 😅

For now though, this PR LGTM! 😋

@zFernand0 zFernand0 requested a review from enamkhan January 8, 2025 16:16
@AndrewTwydell AndrewTwydell force-pushed the lint-fix branch 4 times, most recently from 793ba73 to 4267f18 Compare January 9, 2025 16:09
Copy link
Contributor

@enamkhan enamkhan left a comment

Choose a reason for hiding this comment

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

LGTM!

@AndrewTwydell AndrewTwydell merged commit d7ccd4c into main Jan 9, 2025
19 checks passed
@AndrewTwydell AndrewTwydell deleted the lint-fix branch January 9, 2025 16:52
@AndrewTwydell AndrewTwydell added the release-current Indicates that there is no new functionality being delivered label Jan 9, 2025
@github-actions github-actions bot added released and removed release-current Indicates that there is no new functionality being delivered labels Jan 9, 2025
Copy link

github-actions bot commented Jan 9, 2025

Release succeeded for the main branch. 🎉

The following packages have been published:

Powered by Octorelease 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

4 participants