Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce PublicationAddress.TryParse and switch BasicProperties.ReplyToAddress to use it #831

Merged
merged 9 commits into from
May 12, 2020

Conversation

michaelklishin
Copy link
Member

Proposed Changes

This is an alternative to #828 (with @ig-sinicyn's test case retained for credit) which does
not change the behavior of PublicationAddress.Parse and instead introduces PublicationAddress.TryParse which is then used by BasicProperties.ReplyToAddress.

Types of Changes

  • Bug fix (non-breaking change which fixes issue Rabbitmq 6.0, ReplyToAddress getter throws ArgumentNullException #827)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

…ge-properties-do-not-throw""

This reverts commit ed65854.

See the discussion in #827. We will slightly tweak the approach
but at the very least the test added by @ig-sinicyn can be
retained.
instead of changing the behavior of an existing one.

Per discussion with @bording and @ig-sinicyn in #827.
@michaelklishin michaelklishin changed the title Rabbitmq dotnet client 827 Introduce PublicationAddress.TryParse and switch BasicProperties.ReplyToAddress to use it May 11, 2020
@michaelklishin
Copy link
Member Author

@bording @ig-sinicyn is my interpretation of the Try* idiom here close to what you folks have in mind?

@lukebakken lukebakken self-assigned this May 11, 2020
Copy link
Contributor

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code formatting is very odd ... is this visual studio code?

@michaelklishin
Copy link
Member Author

Ready for another round.

Copy link
Contributor

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 from me. I would appreciate a quick glance from @bording (no hurry!)

@lukebakken lukebakken added this to the 6.1.0 milestone May 11, 2020
Copy link
Collaborator

@bording bording left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a suggestion for a change, but it's just a simplification.

@michaelklishin michaelklishin merged commit 9f0e9f5 into master May 12, 2020
@michaelklishin michaelklishin deleted the rabbitmq-dotnet-client-827 branch May 12, 2020 11:36
michaelklishin added a commit that referenced this pull request May 12, 2020
Introduce PublicationAddress.TryParse and switch BasicProperties.ReplyToAddress to use it

(cherry picked from commit 9f0e9f5)
@lukebakken
Copy link
Contributor

@bording as always, thanks for your input!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants