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

remove dead code #1101

Merged
merged 4 commits into from
Jul 29, 2024
Merged

remove dead code #1101

merged 4 commits into from
Jul 29, 2024

Conversation

terrazoon
Copy link
Contributor

@terrazoon terrazoon commented Jun 20, 2024

Description

The Vulture tool scans for dead code. In our build process, it runs automatically, but we have tuned the confidence level to 100 percent. So if guaranteed dead code exists, the build will fail.

However, it is possible to tune Vulture down to 60 percent confidence, at which point it makes informed guesses about whether code is dead or not. With these findings, a developer has to go through and verify "yes, this code looks dead" but still it is a handy way to find questionable code.

In this PR we remove a few things that are flagged by Vulture and clearly are dead.

Security Considerations

N/A

@terrazoon terrazoon marked this pull request as draft June 20, 2024 15:29
@terrazoon terrazoon self-assigned this Jun 21, 2024
@terrazoon terrazoon linked an issue Jun 21, 2024 that may be closed by this pull request
3 tasks
@terrazoon terrazoon marked this pull request as ready for review June 26, 2024 18:38
@terrazoon terrazoon requested review from ccostino and a team June 26, 2024 18:38
Copy link
Contributor

@xlorepdarkhelm xlorepdarkhelm left a comment

Choose a reason for hiding this comment

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

LGTM, just wondering though, should the code be commented out or removed? We'll have the code existing in git, so wondering if we need to really keep it in the code, as commented out code tends to make things messy.

Copy link
Contributor

@ccostino ccostino 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 going through and gathering up a few things here, @terrazoon!

I agree with @xlorepdarkhelm, let's just straight up remove it. If it's still necessary, it'll be obvious we need to keep it and we can put it back in, but let's not leave it to chance that this all continues to linger.

@ccostino
Copy link
Contributor

We also need an updated PR description with the details of the PR instead of the template, please. 🙂

Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

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

Thanks, @terrazoon!

@ccostino ccostino merged commit 6ba7403 into main Jul 29, 2024
7 checks passed
@ccostino ccostino deleted the notify-api-1002 branch July 29, 2024 16:17
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.

API: Use Vulture to identify potentially unused code
3 participants