WalletConnect / a2

An Asynchronous Apple Push Notification (apns2) Client for Rust
MIT License
136 stars 47 forks source link

Add support for critical alerts #71

Closed Mstrodl closed 11 months ago

Mstrodl commented 1 year ago

Description

How Has This Been Tested?

Due Dilligence

Not sure if I'm filling these out correctly:

Would really love for someone to code review this and let me know if there's anything I can improve on!

Mstrodl commented 1 year ago

@HarryET Bumping this, since it seems like you're the maintainer

HarryET commented 1 year ago

@HarryET Bumping this, since it seems like you're the maintainer

Hey @Mstrodl, yeah I'm maintaining it! I'm OOO for the rest of this week but will do a proper review next week, thanks for the PR 😄

Mstrodl commented 1 year ago

@HarryET Made some changes... Let me know what you think!

HarryET commented 1 year ago

@HarryET Made some changes... Let me know what you think!

Very nice, makes .set_critical() easier to read, do we have tests for that custom serde seralizer/deseralizer?

Mstrodl commented 1 year ago

@HarryET Made some changes... Let me know what you think!

Very nice, makes .set_critical() easier to read, do we have tests for that custom serde seralizer/deseralizer?

Yes, because everything goes through PayloadLike, there shouldn't be any difference. I also have an example of the full thing in the docs, but it's not run automatically because it would need credentials

HarryET commented 1 year ago

@HarryET Made some changes... Let me know what you think!

Very nice, makes .set_critical() easier to read, do we have tests for that custom serde seralizer/deseralizer?

Yes, because everything goes through PayloadLike, there shouldn't be any difference. I also have an example of the full thing in the docs, but it's not run automatically because it would need credentials

Okay, a small test case might be nice but if it's already included by proxy that should be ok

Mstrodl commented 1 year ago

Any news on those reviews?