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

refactor: autopush-common remove legacy autopush #708

Merged

Conversation

taddes
Copy link
Contributor

@taddes taddes commented May 20, 2024

Closes SYNC-4261

This will be somewhat more trivial in scope as SYNC-3451 will remove the dependencies and code related to some of these dependencies (usoto_core, rusoto_dynamodb, and serde_dynamodb)

@taddes taddes changed the title Refactor/sync 4261 autopush common remove legacy autopush Refactor: sync-4261 autopush common remove legacy autopush May 20, 2024
@taddes taddes changed the title Refactor: sync-4261 autopush common remove legacy autopush refactor: sync-4261 autopush common remove legacy autopush May 21, 2024
@taddes taddes changed the title refactor: sync-4261 autopush common remove legacy autopush refactor: autopush-common remove legacy autopush May 21, 2024
@taddes taddes marked this pull request as ready for review May 21, 2024 13:18
@taddes taddes self-assigned this May 21, 2024
@taddes taddes requested review from jrconlin and pjenvey May 21, 2024 20:31
jrconlin
jrconlin previously approved these changes May 21, 2024
Copy link
Member

@pjenvey pjenvey left a comment

Choose a reason for hiding this comment

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

There's a number of ApcErrorKind variants that are no longer used (solely by legacy autopush) that could be removed.

I'm not sure if there's an easy way to detect these other than removing all of them then checking where rustc complains

@taddes
Copy link
Contributor Author

taddes commented May 23, 2024

Great catch @pjenvey , I removed those referenced in autopush by cross referencing the old crate and did some compiler checks to make sure there were no issues. There was an unused Thread(Box<dyn Any + Send>) error that wasn't called anywhere that I took out, too.

@taddes taddes force-pushed the refactor/SYNC-4261_autopush_common_remove_legacy_autopush branch from e8c069b to bbfccc8 Compare May 23, 2024 17:08
@taddes taddes requested a review from pjenvey May 23, 2024 17:08
@pjenvey
Copy link
Member

pjenvey commented May 23, 2024

@taddes I'm seeing even more unused variants!

Sorry to persist on this but this is a good time to remove all the unnecessary ones (and if enough are removed we might even consider removing ApcError itself in the future).

I've spotted PongTimeout and ExcessivePing as being from legacy autopush (the new autoconnect has their own versions of these), so there might be others as well.

@taddes taddes force-pushed the refactor/SYNC-4261_autopush_common_remove_legacy_autopush branch from c644832 to 456767d Compare May 26, 2024 20:59
Copy link
Member

@pjenvey pjenvey left a comment

Choose a reason for hiding this comment

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

Still seeing a couple here that could be removed such as the PongTimeout and ExcessivePing I mentioned

@taddes taddes force-pushed the refactor/SYNC-4261_autopush_common_remove_legacy_autopush branch from 456767d to f5c01dc Compare May 30, 2024 20:18
@taddes taddes requested a review from pjenvey May 31, 2024 18:38
@taddes
Copy link
Contributor Author

taddes commented May 31, 2024

@pjenvey I think that accounts for all of the unused variants. BroadcastError, GeneralError, ConfigError, & PayloadError
 are all used externally directly, with the others covering errors using the From trait, so I left them be. Let me know if that seems sufficient. Thanks!

Copy link
Member

@pjenvey pjenvey left a comment

Choose a reason for hiding this comment

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

I think I'm also seeing the Json and Httparse variants unused, can you double check those?

@taddes
Copy link
Contributor Author

taddes commented May 31, 2024

Hm, might those need to remain for the status method?

@taddes
Copy link
Contributor Author

taddes commented May 31, 2024

Alright, got em @pjenvey !

@taddes taddes requested a review from pjenvey May 31, 2024 21:05
@taddes taddes merged commit 7ade002 into master May 31, 2024
1 check passed
@taddes taddes deleted the refactor/SYNC-4261_autopush_common_remove_legacy_autopush branch May 31, 2024 21:21
@jrconlin
Copy link
Member

🎉

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

3 participants