-
Notifications
You must be signed in to change notification settings - Fork 33
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
Remove some tuple usage #791
base: main
Are you sure you want to change the base?
Conversation
IMO we should avoid |
I monkey patched |
This will conflict with #786 but I like the approach of using a struct instead of an alias. How about I introduced the struct in mr PR instead? |
Totally do not agree. In this case it makes sense, but there are definitely cases where Tuple or NamedTuples is the better option. |
Well, I'm generalizing a bit. But I think that many uses of NamedTuple can be a struct instead. |
@kickster97 and I wanted this in the mqtt branch, but I guess it's no rush. I think this can be done even better by changing some interfaces to require a |
WHAT is this pull request doing?
It's very annoying when tuples are nested in tuples and you need to do
value[0][1]
. This will changeBindingKey
andQueueBinding
fromTuple
tostruct
.Interesting is that
didn't force the order of the values based on type, see
lavinmq/src/lavinmq/vhost.cr
Lines 365 to 372 in 0603aa9
where the orders in line 369 are swapped compared to
QueueBinding
One problem with this refactoring is that
Hash#[](key)
isn't type, which means we don't get any compile time error when we access our binding hashes with the old value, so there's a risk i've missed change the type in one place or two.HOW can this pull request be tested?
Run specs