search

prjseal / SlackBotMessages

A .NET library to enable you to send bot messages in slack
MIT License
55 stars 19 forks source link

Misuse of .Result? #7

Closed p10tyr closed 5 years ago

p10tyr commented 5 years ago

https://github.com/prjseal/SlackBotMessages/blob/6d7fe818abe60bdb5b25cb699aef41b9fcbcaefc/SlackBotMessages/SbmClient.cs#L57

Hi - I used your package to hit the ground running to integrate some test logging into slack. A colleague of mine noticed this line referenced. we were wondering if there was a specific reason you used .Result instead of await pattern?

There is a situation when running in a IIS pipeline on a NET472 build this will cause a deadlock on this code without ConfigureAwait(false);

prjseal commented 5 years ago

Thanks for raising this @ppumkin

There's no specific reason why I did that over any other way. Please feel free to submit a PR with it working how you suggested above and I can release a new version for everyone.

Cheers

Paul

jamietwells commented 5 years ago

Can you give me permission to push a branch and I'll submit a PR?

prjseal commented 5 years ago

Hi Jamie If you fork and then create a branch on your fork you can submit that as a PR.

Thanks

Paul

jamietwells commented 5 years ago

Fixed that and a couple of other bits too:

https://github.com/prjseal/SlackBotMessages/pull/8

prjseal commented 5 years ago

Thank you I’ve seen the PR I’ll review and merge tomorrow

jamietwells commented 5 years ago

Did you manage to review the PR?

prjseal commented 5 years ago

PR reviewed, tested and merged, thank you very much @jamietwells

prjseal commented 5 years ago

I had to add the previous way of sending back in because of breaking changes. I kept your work in, but just named them as SendAsync, ProcessRequestAsync and BasicTestsAsync.

New version on it's way to NuGet now.

Thanks

Paul