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

Highlight unused code in Pull Requests as a warning #372

Closed
wants to merge 16 commits into from

Conversation

Iron-Ham
Copy link
Contributor

@Iron-Ham Iron-Ham commented May 25, 2024

This pull request adds a new action which runs Periphery to identify unused code within a PR.

Screenshot 2024-05-25 at 2 32 15 PM

Copy link

netlify bot commented May 25, 2024

👷 Deploy request for apollo-ios-docc pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 864852b

Copy link

netlify bot commented May 25, 2024

👷 Deploy request for eclectic-pie-88a2ba pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 864852b

@calvincestari
Copy link
Member

Looks like the new job didn't get triggered in this PR, I'd like to see how long it takes to execute. Is this solving a specific problem you've noticed or just a general codebase hygiene tool you use elsewhere?

@Iron-Ham
Copy link
Contributor Author

Apologies for the late response @calvincestari, travel will do that 😓

It ran here: https://github.com/apollographql/apollo-ios-dev/actions/runs/9237413673/job/25414361878 and took about two minutes. It could be optimized further if we cached the Periphery install from Homebrew. I've set it so that it only runs on PRs that include Swift changes.

Is this solving a specific problem you've noticed or just a general codebase hygiene tool you use elsewhere?

A bit of both! There isn't a ton of unused code in Apollo (at least, with the configuration I've set in the yml file), but there is some. As far as the tool being used elsewhere, I've seen it used locally in various projects – but decided to automate it on CI via Actions shortly before leaving for vacation.

I would argue that before we consider merging this, we should update to the new release which includes the Actions formatter so that we don't have to pipe this through a ruby script at all.

@calvincestari
Copy link
Member

I can see the value in it but I don't think this is a major issue in the codebase. I'm not super keen to have this enforced in every PR but wouldn't mind if it were moved to be a git hook instead so that whoever wants to can install and run locally for their PRs.

@Iron-Ham Iron-Ham closed this Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants