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

Remove some tuple usage #791

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Remove some tuple usage #791

wants to merge 2 commits into from

Conversation

spuun
Copy link
Member

@spuun spuun commented Sep 25, 2024

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 change BindingKey and QueueBinding from Tuple to struct.

Interesting is that

alias QueueBinding = Tuple(BindingKey, Exchange)

didn't force the order of the values based on type, see

def queue_bindings(queue : Queue)
iterators = @exchanges.each_value.map do |ex|
ex.queue_bindings.each.select do |(_binding_args, destinations)|
destinations.includes?(queue)
end.map { |(binding_args, _destinations)| {ex, binding_args} }
end
Iterator(QueueBinding).chain(iterators)
end

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

@spuun
Copy link
Member Author

spuun commented Sep 25, 2024

IMO we should avoid Tuple and NamedTuple everywhere.

@spuun
Copy link
Member Author

spuun commented Sep 25, 2024

I monkey patched Hash locally and didn't find anything related to this change.

@snichme
Copy link
Member

snichme commented Sep 25, 2024

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?

@carlhoerberg
Copy link
Member

IMO we should avoid Tuple and NamedTuple everywhere.

Totally do not agree. In this case it makes sense, but there are definitely cases where Tuple or NamedTuples is the better option.

@spuun
Copy link
Member Author

spuun commented Sep 25, 2024

IMO we should avoid Tuple and NamedTuple everywhere.

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.

@spuun
Copy link
Member Author

spuun commented Sep 25, 2024

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?

@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 BindingKey instead of constructing it everywhere like I did now.

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

Successfully merging this pull request may close these issues.

4 participants