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

Hide the req_packed_commands from docs. #1020

Merged
merged 1 commit into from
Jan 13, 2024
Merged

Conversation

nihohit
Copy link
Contributor

@nihohit nihohit commented Jan 9, 2024

req_packed_commands is meant for internal usage by the Pipeline object, and is an easy to misuse footgun.
See #1017 (comment)

`req_packed_commands` is meant for internal usage by the `Pipeline`
object, and is an easy to misuse footgun.
See redis-rs#1017 (comment)
@nihohit nihohit mentioned this pull request Jan 9, 2024
22 tasks
@jaymell
Copy link
Contributor

jaymell commented Jan 10, 2024

What if we just update the docs to better explain the parameters and otherwise warn against using the method, as you've done here -- I wonder if that will accomplish the goal without hiding it completely? I'm not sure how likely it is, but it seems like hiding it might frustrate downstream users/libraries unnecessarily.

@nihohit
Copy link
Contributor Author

nihohit commented Jan 10, 2024

As far as I understand the #[doc(hidden)] command, it just hides this from generated documentation. It doesn't prevent usage of this function - hence, no semver-breaking tag on this PR. So, I'm not sure what might cause frustration here - users who already use this function will be able to keep using it, and those that don't (and probably shouldn't) will have a harder time being aware of the function. Anyone who wants to implement ConnectionLike will still be prompted by the compiler to implement these functions.
I don't have a strong opinion on this, though. If you strongly believe that we should limit this change to just a documentation update, I'm fine with that.

@jaymell
Copy link
Contributor

jaymell commented Jan 13, 2024

I don't have a strong opinion on this, though. If you strongly believe that we should limit this change to just a documentation update, I'm fine with that.

I don't have a real strong opinion either, but it feels a bit wrong to me to hide methods of public traits from the documentation. I understand the reasoning though, so not opposed to it.

@nihohit nihohit merged commit 14b460c into redis-rs:main Jan 13, 2024
10 checks passed
@nihohit nihohit deleted the footgun branch January 17, 2024 21:29
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.

None yet

2 participants