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

Adds a script for removing unused diagnostics #44324

Merged
merged 3 commits into from
Aug 24, 2021

Conversation

orta
Copy link
Contributor

@orta orta commented May 28, 2021

This adds a script which only runs on linux boxes, that searches the codebase for every diagnostic and lets you know if nothing is found.

TODO: Add to CI.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 28, 2021
@orta
Copy link
Contributor Author

orta commented Jun 3, 2021

Screen Shot 2021-06-03 at 4 43 32 PM

I can use findstr to check

const diagName = line.split(":")[0].trim();

try {
execSync(`grep -rnw 'src' -e 'Diagnostics.${diagName}'`).toString();
Copy link
Member

Choose a reason for hiding this comment

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

there's probably a platform-independent package on npm that does recursive file string search, although feature-detecting grep vs findstr would work too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wes thinks ripgrep ( https://www.npmjs.com/package/vscode-ripgrep ) should be a reasonable tradeoff here

Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid the offensive huge cost of this you could just grep all lines that include Diagnostics., print them to a temp file, then search through this file. Also, even if the names are well behaved, it would be better to add -F. Also, -n is redundant AFAICT.

(And doing it with a grep library seems like an overkill too...)

@@ -5793,10 +5617,6 @@
"category": "Message",
"code": 95001
},
"Convert function '{0}' to class": {
Copy link
Member

Choose a reason for hiding this comment

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

I'm almost certain this message and the ones that follow are used in codefixes. I wonder why our fourslash tests don't fail with them gone.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Well...early in a release's development is the best time to try something like this.

@orta you want to try merging this early on Monday to see whether it breaks anything?

(I think @elibarzilay 's right about grep -- almost all the team can run this script in an environment with grep if they need to.)

@orta
Copy link
Contributor Author

orta commented Aug 24, 2021

Yeah, lets do it

@orta orta merged commit 41dcad0 into microsoft:main Aug 24, 2021
BobobUnicorn pushed a commit to BobobUnicorn/TypeScript that referenced this pull request Oct 24, 2021
* Adds a script for removing unused diagnostics

* Accidentally deleted the wrong one
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants