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

Improve Knip config #2726

Merged
merged 1 commit into from
Jan 14, 2024
Merged

Improve Knip config #2726

merged 1 commit into from
Jan 14, 2024

Conversation

webpro
Copy link
Contributor

@webpro webpro commented Jan 14, 2024

After being very glad to see Knip being used in this repository, I saw the configuration and the slightly uncommon blend of the vscode and agent workspaces ("../agent/**/*.ts"), I couldn't resist to check out the repo and come up with some suggestions, as presented in this PR.

Looks like most of the results are (still) correct and there's even more to clean up, yay! I'm happy to leave the actual clean up to developers more familiar with the codebase (I'm also unaware of potential external factors/consumers).

Feel free to AMA!

Additionally, Knip v4 is around the corner and will give a nice speed bump (on my machine it goes down from 4.7s to 2.8s with the same config).

Test plan

CI

@webpro
Copy link
Contributor Author

webpro commented Jan 14, 2024

By the way, looks like including unused class members to the report gives clues to remove more dead code :)

@sqs
Copy link
Member

sqs commented Jan 14, 2024

@webpro Awesome, thanks! This fixes some of the false positives. Appreciate the PR.

@sqs sqs enabled auto-merge (squash) January 14, 2024 08:39
@sqs sqs disabled auto-merge January 14, 2024 09:03
@sqs sqs merged commit 3643c8d into sourcegraph:main Jan 14, 2024
12 of 15 checks passed
@sqs
Copy link
Member

sqs commented Jan 14, 2024

Ran e2e tests locally to merge. Seems to be an auth problem on GitHub Actions.

@webpro webpro deleted the knip-config-suggestions branch January 14, 2024 09:10
@sqs sqs mentioned this pull request Jan 14, 2024
@sqs
Copy link
Member

sqs commented Jan 14, 2024

@webpro Indeed, I found a bunch more unused code to delete: #2723. Thank you!!

I noticed a couple false positives:

Unused exported class members (20)
providedCodeActionKinds  DocumentCodeAction      vscode/src/code-actions/document.ts:7:28                        
providedCodeActionKinds  EditCodeAction          vscode/src/code-actions/edit.ts:6:28                            
providedCodeActionKinds  ExplainCodeAction       vscode/src/code-actions/explain.ts:4:28                         
providedCodeActionKinds  FixupCodeAction         vscode/src/code-actions/fixup.ts:12:28                          
getContext               FilenameContextFetcher  vscode/src/local-context/filename-context-fetcher.ts:32:18  

But neither are shown by tsserver in Find References either, so it's not knip's fault and it's not something knip should reasonably expect to hack around. So IMO knip works 100% as expected!

@webpro
Copy link
Contributor Author

webpro commented Jan 14, 2024

Thanks for pointing out the issues. Tbh, I'm not 100% convinced they are false positives (if I follow the code paths then they don't seem to be actually used). But yeah if Find References won't find it then (the TS Language Server internal to) Knip won't either.

sqs added a commit that referenced this pull request Jan 14, 2024
Unused as reported by [knip](https://knip.dev) and verified by me
through find-references and search. Made possible by the broader knip
config in #2726 (thanks,
@webpro!).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants