Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Sep 16, 2021

Resolves #271

Channel#set_options (RSL7) was implemented as Channel#update_options.
Added aliases: Channel#options= and Channel#set_options

@ghost ghost self-assigned this Sep 16, 2021
@ghost ghost requested a review from owenpearson September 20, 2021 09:02
Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

Actually, I'm not 100% sure on this. @lukaszsliwa What's the reason for the second alias? And can we emit a deprecation warning for people using update_options so that we can give people a chance to migrate to set_options and then we can remove update_options in the next major release.

@ghost
Copy link
Author

ghost commented Sep 20, 2021

Actually, I'm not 100% sure on this. @lukaszsliwa What's the reason for the second alias? And can we emit a deprecation warning for people using update_options so that we can give people a chance to migrate to set_options and then we can remove update_options in the next major release.

I added second alias because it is more natural for ruby but spec says that it should be set_options.

Copy link
Contributor

@TheSmartnik TheSmartnik left a comment

Choose a reason for hiding this comment

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

While set_options isn't very rubyish it adheres to the doc and options= is an idiomatic ruby method. We do add aliases purely for convenience so I guess it's a good idea

@owenpearson owenpearson merged commit 0943825 into main Sep 27, 2021
@owenpearson owenpearson deleted the RSL7 branch September 27, 2021 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Add support for RSL6b, RLS7 (Channels)

2 participants