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

Adds a type-erased response adapter to the public API #201

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

anarthal
Copy link
Collaborator

@anarthal anarthal commented Jun 9, 2024

Would something along these lines suit you @mzimbres? If it does, I'll clean it up and write docs & tests for it.

close #128

@mzimbres
Copy link
Collaborator

mzimbres commented Jun 9, 2024

@anarthal Thanks, see my comments.

@anarthal
Copy link
Collaborator Author

anarthal commented Jun 9, 2024

Did you publish your comments? I can't see any of them

include/boost/redis/detail/connection_base.hpp Outdated Show resolved Hide resolved
include/boost/redis/connection.hpp Outdated Show resolved Hide resolved
@mzimbres
Copy link
Collaborator

mzimbres commented Aug 4, 2024

Are you still going to work on this PR?

@anarthal
Copy link
Collaborator Author

anarthal commented Aug 4, 2024

whoa, I completely forgot about this! Yes, I'll finish it this incoming week.

@anarthal
Copy link
Collaborator Author

anarthal commented Aug 7, 2024

@mzimbres this is now complete on my side. Please let me know if any changes are required.

@mzimbres
Copy link
Collaborator

mzimbres commented Aug 7, 2024

Many thanks! Give me some days to review it. I would also say you this is worth listing in the Changelog.

}

template <class Executor>
friend class detail::connection_base;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there should be no friend class here, the class should define

void operator(std::size_t, resp3::basic_node<std::string_view> const&, system::error_code&)

and forward the call directly to impl_. Once you do that you can also remove this typedef and replace it with any_adapter AFAICS.

In a later issue we will have to think about how this relates to the receiver adapter or whether we need another any adapter class. This will be needed to type erase the receive operation too

Copy link
Collaborator Author

@anarthal anarthal Aug 11, 2024

Choose a reason for hiding this comment

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

This makes sense only if any_adapter is a public class - otherwise I can remove it (together with the Doxygen docs). See my question above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, let us keep it public.

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.

Move the adapter module to the detail namespace
2 participants