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

Make detail tuples typed-ish #837

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

Make detail tuples typed-ish #837

wants to merge 14 commits into from

Conversation

spuun
Copy link
Member

@spuun spuun commented Nov 9, 2024

WHAT is this pull request doing?

"detail tuples" are NamedTuples which causes a lot of limitations and struggle when refactoring.

One approach to mitigate this would have been to declare a lot of structs for every case we need, but that would require a lot of work and make the code less flexible.

This PR will introduce Metadata which is a wrapper around NamedTuple. This makes it possible to add helper methods on the details instead of doing a lot of "workarounds" where the data is accessed. Looking at the diff for src/lavinmq/http/controller.cr one will see that the code is simplified.

Values is wrapped in Metadata::Value which is for Metadata what JSON::Any is to JSON.

One thing that maybe needs to be thought through and fixed is Value#<=>. It will probably almost always compare to values of the same type and thereby compare correct, but other edge cases maybe needs to be changed.

HOW can this pull request be tested?

Run specs.

Copy link
Member

@carlhoerberg carlhoerberg left a comment

Choose a reason for hiding this comment

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

can you show how this will simplify something in a controller?

src/lavinmq/http/controller.cr Outdated Show resolved Hide resolved
src/lavinmq/amqp/channel.cr Outdated Show resolved Hide resolved
src/lavinmq/metadata.cr Show resolved Hide resolved
@spuun
Copy link
Member Author

spuun commented Nov 11, 2024

can you show how this will simplify something in a controller?

It simplifies Controller#page method.

@carlhoerberg
Copy link
Member

can you show how this will simplify something in a controller?

It simplifies Controller#page method.

ok, i see it now. it's less code in Controller#page but does it have other benefits, like more correct, doesn't have to load all data to sort etc?

@carlhoerberg
Copy link
Member

It's not like Metadata::Value#fetch is easily digestible, so in a sense one can say that the complexity has just been pushed around and we've added more indirection. That is if there are no other benefits that I don't see.

@spuun
Copy link
Member Author

spuun commented Nov 11, 2024

ok, i see it now. it's less code in Controller#page but does it have other benefits, like more correct, doesn't have to load all data to sort etc?

Not yet, but it enables other refactoring to improve it.

One purpose of this is to simplify the abstraction of different entities. I've struggled with detail_tuples and all of the Iterators when trying to abstract first Client and now Channel. I'm just trying to decouple in smaller steps.

It's not like Metadata::Value#fetch is easily digestible, so in a sense one can say that the complexity has just been pushed around and we've added more indirection. That is if there are no other benefits that I don't see.

The "tree traversing" is a little bit of a mess because of macro language limitations, but the generated code should be straight forward.

Moving the logic also makes it easier to test it.

src/lavinmq/metadata.cr Outdated Show resolved Hide resolved
src/lavinmq/data_dir_lock.cr Outdated Show resolved Hide resolved
src/lavinmq/metadata.cr Outdated Show resolved Hide resolved
@kickster97 kickster97 mentioned this pull request Nov 18, 2024
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.

3 participants