Closed gaelcolas closed 8 years ago
Awesome, thanks Gael! Looks good to me, any objections @gpduck?
Hi all!
Apologies, I've been out of commission on vacation - any issues with merging this? cc @gpduck @gaelcolas
Thanks!
Warren
sorry, I've been travelling. I'll try cleaning this off asap.
On Wednesday, August 17, 2016, Warren F. notifications@github.com wrote:
Hi all!
Apologies, I've been out of commission on vacation - any issues with merging this? cc @gpduck https://github.com/gpduck @gaelcolas https://github.com/gaelcolas
Thanks!
Warren
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RamblingCookieMonster/PSRabbitMq/pull/7#issuecomment-240516929, or mute the thread https://github.com/notifications/unsubscribe-auth/AIjANRSoffmYB3W4BY98etMiUZ6UKwPZks5qg124gaJpZM4JZHNv .
Hey @gpduck, I think I've fixed all the above. Would you mind reviewing and merge if all ok?
Thanks :)
Looking good.
I see you set the default parameter set on Send-RabbitMqMessage to SerlializeAs, but didn't put SerializeAs and ContentType into separate parameter sets. Was there a reason for leaving it so they could both be specified?
Oh yeah, it was for backward compatibility mainly, and more flexibility. You want previous scripts to work without SerializeAs set but with ContentType set, with same behavior as before (which means default SerializeAs CliXml). Also, you may want to serializeAs something, but with a different contentType field in the BasicProperties for your own de-serialization. So in the end I opted for same parameter Set, and ContentType always overriding the BasicProperty, and SerializeAs, setting the ContentType if it wasn't explicitly specified.
It's the drawback of option 2, but at least backward compatible. We can revert to option 1 in subsequent versions...
Just fixed something actually about that behaviour. Automatic De-serialization was broken in some cases because the Content-Type property was not set. I should really add more unit and integration test... one day!
@RamblingCookieMonster could you please do your magic? :)
Awesome, thanks for all the work @gaelcolas and the feedback @gpduck! Merging this in
Hi @gpduck and @RamblingCookieMonster,
Here's a PR to add some features I needed (should not be any breaking change):
I've written some Integration tests that I run through Test-Kitchen, but it'd mess-up your current scaffolding, so I've excluded them via .gitignore for now. This system needs to mature a bit before I share it.
Feedback welcome. :)