WalletConnect / a2

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

Add Deserialize impls, a new Client constructor, and some QoL features #49

Closed AzureMarker closed 1 year ago

AzureMarker commented 3 years ago
AzureMarker commented 3 years ago

@pimeys Can you look into merging these PRs? We currently have to rely on a fork.

pimeys commented 3 years ago

Hey! Sorry for delay, I've been (and still will be) on vacation with only an old openbsd laptop with me. I can try to test and release a new version, but it might be complex...

pimeys commented 3 years ago

So... I'll be back to Berlin to my work computer on 26.8. I don't have a good enough computer to review and test this. Could some other long-time a2 user like @dbrgn maybe see if this makes sense?

AzureMarker commented 3 years ago

@pimeys Are you still interested in these changes?

jrconlin commented 3 years ago

Is this PR blocked? I see that there are changes requested, are those still valid?

pimeys commented 3 years ago

As you probably noticed, I'm quite busy currently and not really having time on older crates that I'm not working with anymore. Sorry about that. I'm still not convinced we should make the fields of the struct public. And also not using this crate anymore, but I know there are active users such as @dbrgn, who'd be much better saying should we change the API or not.

dbrgn commented 3 years ago

I haven't touched the codebase that uses a2 in a while. I may be able to take a look in early January (I made a note for that), but at the moment I'm quite busy as well :slightly_smiling_face:

pimeys commented 3 years ago

Haha, no worries! Like, also first vacation for me in ages. I'm a bit worried merging and touching a codebase I'm not actively using anymore, especially when it changes a public-facing api...

dbrgn commented 3 years ago

Yeah, and I'm wary of updating dependencies on a service that's been running perfectly fine for months :sweat_smile:

However, I should be able to judge the API changes independently from that (this PR only changes the API, not how network events are handled or anything like that).

pimeys commented 3 years ago

What, you aren't excited about upgrading to Tokio 0.3 or 1.0 already?-)

Jokes aside, what would be cool to do here is what we did in Tiberius, make it runtime-independent. Although it's quite off-topic already... :D

jrconlin commented 3 years ago

heh, well, first off, FOR THE LOVE OF WHATEVER YOU DEEM HOLY, PLEASE GO ENJOY YOUR BREAKS!

(Sorry, I do a lot of back end work and may be a bit triggered about seeing folks doing work when there's other, more enriching things to do.)

Next: Honestly, this gives me a lot more info to work with. We're using this library as part of our back-end service. We've already vendored a fork of it, so no worries about any blocks. Also totally fine if y'all treat this as semi-abandonware or archive it. I kinda had the feeling that was the case anyway.

In anycase, go, shoo, have a festive Yule, Happy Saturnalia, Merry Christmas, Joyous Hanukkah, reasonably OK Festivus or whatever else y'all want to have, and all the best in the New Year.

pimeys commented 3 years ago

If you made a fork of it, what about taking the ownership of the whole crate?

jrconlin commented 3 years ago

Heh, yeah, no. We have enough stuff https://github.com/mozilla-services we have to support for a team of three engineers.

I mean, it's public in our repos https://github.com/mozilla-services/a2, and we'll do regular updates on it so long as we use it, of course, but we're probably not going to address anything in the issue backlog.

I suppose we could clean up the inheritance chain a bit to make the link a bit more direct, and I'm going to guess we should probably legacy the "Contact" info if you want... Ok, let me at least talk it over with the rest of the team in January and see what we might be able to do, but we also might just switch off to a different library if we find one.

On Tue, Dec 22, 2020 at 8:41 AM Julius de Bruijn notifications@github.com wrote:

If you made a fork of it, what about taking the ownership of the whole crate?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/pimeys/a2/pull/49#issuecomment-749645857, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAIXK7FLCOOAXWT7S2UZCTSWDD4NANCNFSM4PG6RZ5A .

pimeys commented 3 years ago

I mean, I doubt there will be that much of changes to a2, at least when tokio stabilizes finally.

We did the same thing as an organization to tiberius, basically rewrote it and negotiated with the original author to get access to the crates.io crate and make our fork the main one. I'd be willing to do the same for this and my fcm + web-push crates, if there'd be an interested community keeping them alive and moving.

HarryET commented 1 year ago

👋🏼 The project has new maintainers now. I'm getting up-to speed with everything and trying to tidy up the PRs and Issues; are the changes here still relevant?

jrconlin commented 1 year ago

@HarryET Welcome! (And thank you!)

Yes, these are still valid, in that we still use them in our fork and it might improve things for others.

HarryET commented 1 year ago

Hey @jrconlin, thanks! Can you move the PR to point into v0.7 instead of master? and I'll get these reviewed asap.

jrconlin commented 1 year ago

hrm... I don't have access to AsureMarker's account, so I can't post from that. I'll close this PR and open a new one from ours.

HarryET commented 1 year ago

@jrconlin looks like I can switch it don't worry. But if it would be easier long term I can close this and work on a new PR with you?

jrconlin commented 1 year ago

Yeah, I think it would.

Mark did this original work while he was an intern. He's now off to better things. The other repo has shared ownership.

HarryET commented 1 year ago

Yeah, I think it would.

Mark did this original work while he was an intern. He's now off to better things. The other repo has shared ownership.

👌🏼 I'll close this out and will review a new PR once open