Skip to content

Conversation

@andy-stark-redis
Copy link
Contributor

We had existing code samples for the JSON datatype pages in the redis-py repo but the doc page wasn't set up to use them. Both the doc page and the Python code needed fixes to get them into shape. The Python stuff is already accepted and merged into redis-py and this PR contains the corresponding doc page changes.

@andy-stark-redis andy-stark-redis added the documentation Improvements or additions to documentation label May 16, 2024
@andy-stark-redis andy-stark-redis requested a review from a team May 16, 2024 09:56
@andy-stark-redis andy-stark-redis self-assigned this May 16, 2024
@andy-stark-redis
Copy link
Contributor Author

The staging link for the path page shows glitches for some of the Filter examples but they work in a local build and I can't find anything wrong with the syntax :-(

Copy link
Collaborator

@dwdougherty dwdougherty left a comment

Choose a reason for hiding this comment

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

These need some work. In some cases, answers differ between redis-cli and Python. In other cases, the entire script is displayed instead of just the required snippet (this is usually a problem with mismatched tags in the Python code).

Let's hop on a Slack call @andy-stark-redis and go through them.

@andy-stark-redis
Copy link
Contributor Author

Shortening the CLI examples seems to have fixed the Python examples for some reason...

@dwdougherty
Copy link
Collaborator

Weird.

with [`JSON.GET`]({{< baseurl >}}/commands/json.get/):

```sh
{{< clients-example cli only >}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious: what was the impetus for this change? If an example is clearly CLI only, as this one is, I don't think it makes sense to use the clients-example shortcode. Change my mind. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:-D I don't know if it will change your mind or not but I thought it was helpful to see the legend "Redis CLI" explicitly at the top given that most samples in the page are for both CLI and other languages. I can remove it if it's out of step with what we do elsewhere.

@dwdougherty
Copy link
Collaborator

Sorry @andy-stark-redis, but we need to chat about this some more. I'll try to catch you before tomorrow afternoon's meetings.

Copy link
Collaborator

@dwdougherty dwdougherty left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @andy-stark-redis, for being patient with all the back and forth! :)

@andy-stark-redis
Copy link
Contributor Author

Not at all, @dwdougherty ! Thanks very much to you for patiently guiding me to the right solution :-)

@andy-stark-redis andy-stark-redis merged commit e4e27a7 into main May 20, 2024
@andy-stark-redis andy-stark-redis deleted the DOC-3787-enable-json-samples branch May 20, 2024 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants