search

bytefish / FcmSharp

Firebase Cloud Messaging (FCM) with .NET
MIT License
6 stars 7 forks source link

Better messaging error response, some documentation #38

Closed sir-boformer closed 5 years ago

bytefish commented 6 years ago

Good work! 😎 Please give me some time to take a look, before merging it.

sir-boformer commented 6 years ago

of course. I figured out a way with the existing Dictionary

bytefish commented 6 years ago

Just a question, I am reading the diff on the phone so I might be wrong. 😅

Why is the content passed twice into the Exception:

https://github.com/bytefish/FcmSharp/blob/d7f642241f6423b0c72f1fc9b47555bf8d47b9e4/FcmSharp/FcmSharp/FcmClient.cs#L98

Besides this everything looks good! 🤜🤛

Maybe you can update the Pull Request and I will merge it. 👍

bytefish commented 6 years ago

@sir-boformer If I merge it, what do you think about the version number of the library? Should it be a 3.0 then? Because some people might have built their error handling on the previous implementation, so this looks like a breaking change.

sir-boformer commented 6 years ago

Makes sense.

Also, about passing the content twice. That's because I wanted a dedicated field for the raw response, so that people can parse it manually (first argument). The second argument is the human-readable message, which is currently also the raw response, but could also be something else.

bytefish commented 6 years ago

Please don't be disappointed for me not merging yet! I will for sure, but I am hesitant to make breaking changes and go to 3.0 for now.

bytefish commented 5 years ago

@sir-boformer Thank you very, very much for your Pull Request. I always considered merging it in the past months, but I am still hesitant... just because I still don't know if I am interpreting the protocol correctly. This sucks a lot for users of the API... but at the same time I don't want API requests to fail, because I missed something in the docs and the deserialization fails. Sorry for closing and thanks again for your good support!