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

src: Delete unused code in uninstall.js #104

Merged
merged 3 commits into from
Dec 15, 2023

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Dec 13, 2023

This is a quick follow-up to some code removal in PR 103.

In #103, after removing some code that does a ping to the backend (which we no-op/ignore anyway, on the backend side...) I didn't notice packageVersion is now defined but unused with those code removals.

Basically this is one line I missed in the cleanup. Things that were part of making the backend ping work. With that removed, this line/variable serve no purpose.

This basically finishes the cleanup started in #103.

(TL;DR: Another too-long writeup of a tiny diff. This is a one-liner removing a defined but unused var.)

EDIT: Whoops, mentioned and tagged the wrong PR. This PR follows-up on PR 103, not PR 95...

Follow-up to some code removal in PR 103.
@DeeDeeG DeeDeeG force-pushed the cleanup-delete-unused-variable branch from 3828b37 to cb47127 Compare December 13, 2023 06:15
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Dec 13, 2023

macOS CI failure is unrelated, it's the "old node-gyp, new Python" issues again.

I have to review how we worked around these things in CI for other repos. Wanted to get this PR out quickly, will have to come back around to fixing CI here soon.

@confused-Techie
Copy link
Member

Taking a quick look here, so feel free to tell me if I'm wrong.

But I think the class utilized in this function getPackageVersion is also totally unused. So we should be able to delete line 30-36 as well.

Otherwise diff looks good and just like we would expect

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Dec 14, 2023

Wow, you're right, getPackageVersion can go too! This file has to be almost half the length it was before, with all this pruning. [EDIT: Okay, not quite that proportion of code was removed, but a sizeable chunk. 34 lines pruned, whereas the file is now 79 lines long.]

@DeeDeeG DeeDeeG changed the title src: Delete unused variable in uninstall.js src: Delete unused code in uninstall.js Dec 14, 2023
@confused-Techie
Copy link
Member

@DeeDeeG Thanks for taking a look and confirming what I saw there.
More good news, now with that function removed, it looks like we can stop requiring season in this file as well!

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Dec 15, 2023

With that removal, I'm reasonably confident-ish that there's no other dead/unused code that can simply be deleted from this file. Tempting fate with that, but I think this is good now(?).

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Sorry I kept adding onto this one.

But yup I agree that it seems everything has been taken from this file that we can for now.

In which case it looks good to me, and lets get it merged!

Thanks for putting this one together, and not minding me adding onto it

@confused-Techie confused-Techie merged commit 571e2ca into master Dec 15, 2023
11 checks passed
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Dec 15, 2023

No problem! Good to be thorough.

Thanks for the reviews/feedbacks.

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.

2 participants