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

GEODE-9999: Update the Geode for Redis documentation #7326

Merged
merged 4 commits into from Feb 2, 2022
Merged

GEODE-9999: Update the Geode for Redis documentation #7326

merged 4 commits into from Feb 2, 2022

Conversation

jomartin-999
Copy link
Contributor

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • Is your initial contribution a single, squashed commit?

  • Does gradlew build run cleanly?

  • Have you written or updated unit tests to verify your changes?

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

@jdeppe-pivotal jdeppe-pivotal changed the title GEODE-9999-Update the Geode for Redis documentation GEODE-9999: Update the Geode for Redis documentation Jan 28, 2022
@jdeppe-pivotal jdeppe-pivotal added the redis Issues related to the geode-for-redis module label Jan 28, 2022
@davebarnes97
Copy link
Contributor

davebarnes97 commented Jan 28, 2022

@jomartin-999 Since I'm reviewing for style & format, rather than functionality, I'll defer my review until you've addressed Donal's concerns. (And congratulations on the very cool JIRA ticket number.)

Copy link
Contributor

@nonbinaryprogrammer nonbinaryprogrammer left a comment

Choose a reason for hiding this comment

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

There's one formatting change that I think is important to the readability of the docs. Also this isn't something that you changed, but the note on line 219 does not say anywhere in it that it is for the HSCAN command. Obviously it is a footnote of the HSCAN command so it isn't like that info is missing, but all the other footnotes start with the command they are talking about, and I think consistency is nice.

@DonalEvans
Copy link
Contributor

There's one formatting change that I think is important to the readability of the docs. Also this isn't something that you changed, but the note on line 219 does not say anywhere in it that it is for the HSCAN command. Obviously it is a footnote of the HSCAN command so it isn't like that info is missing, but all the other footnotes start with the command they are talking about, and I think consistency is nice.

This footnote applies to HSCAN, ZSCAN and SSCAN, not just HSCAN, so if this change is made, it should mention all three commands.

Copy link
Contributor

@nonbinaryprogrammer nonbinaryprogrammer left a comment

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor

@davebarnes97 davebarnes97 left a comment

Choose a reason for hiding this comment

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

+1 LGTM

@DonalEvans DonalEvans merged commit 064fe00 into apache:develop Feb 2, 2022
DonalEvans pushed a commit that referenced this pull request Feb 2, 2022
@jomartin-999 jomartin-999 deleted the update-geode-for-redis-doc branch February 2, 2022 21:11
mhansonp pushed a commit to mhansonp/geode that referenced this pull request Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
redis Issues related to the geode-for-redis module
Projects
None yet
6 participants