Skip to content

Conversation

dwdougherty
Copy link
Collaborator

@dwdougherty dwdougherty commented Aug 26, 2025

Hi @andy-stark-redis. This PR is an experiment to see if Augment can successfully create TCEs in all currently supported languages. At some point, the examples in the local_examples/cmd_exists/LANG directories can become PRs for each LANG's repo.

I don't have the correct system setup to test each of these; hoping you help here (at some point--no rush). Maybe you can just advise on setup so I can test new examples myself.

Copy link
Contributor

github-actions bot commented Aug 26, 2025

DOC-5630

Copy link
Contributor

@dwdougherty dwdougherty marked this pull request as draft August 26, 2025 17:15
Copy link
Contributor

@andy-stark-redis andy-stark-redis left a comment

Choose a reason for hiding this comment

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

This is shaping up really well! The code basically looks OK and I compiled and ran a few of the files and they worked fine. The main issue is the test suite stuff that appears in the original sources, but gets extracted from the examples.

A few thoughts:

  • Just out of interest, did you refer Augi... Augment to the examples on the website or to the examples in the client repos for the languages? I've found it tends to do better when you explicitly say "look at the examples in doctests", but the downside is that you might have to do this separately for each language, which I guess is not the goal here.
  • I'd be interested to know if you selected Claude or GPT5 for this. I'm still trying to get an impression of both but Claude seems faster and tries to be "fun" (emojis and 'You're absolutely right!", etc), while GPT is slower and boring but seems to think more deeply about what it's doing. Claude might actually be better for short snippets like the ones in the command pages.
  • I don't know if you are using Enhance Prompt, but when you do, it usually gives you a list of requirements for the conversion. It's then quite easy to add your own short directions and reminders to the list ("convert all the examples, not just some of them", "translate the tests as well as the snippets", etc).

Really good, though - great bit of exploration here :-)

@dwdougherty dwdougherty self-assigned this Sep 10, 2025
@dwdougherty dwdougherty marked this pull request as ready for review September 11, 2025 20:04
@dwdougherty
Copy link
Collaborator Author

dwdougherty commented Sep 11, 2025

Hi @andy-stark-redis. I manually restructured the invalid examples using code I found in other examples. I'm still not set up to test C#, GO, Java/Lettuce, and PHP, so this is a Hail Mary pass. When you have time, would you please take a look?

@andy-stark-redis
Copy link
Contributor

This is really getting somewhere now :-) Still a few minor glitches, but easy to fix and you'd expect them to disappear as we add more of these examples for it to use as a reference. I'd say it would be more or less OK to use now, as long as we are careful about checking the output and fixing it (but we are expecting to have to do that anyway, right?)

Summary is below:

NRedisStack: looks fine.

Predis:
- Tries to create an instance of Client (class is actually declared as PredisClient)
- $existsResult5 = $r->exists(['key1', 'key2', 'nosuchkey']); uses an array of keys but it actually takes varargs: $existsResult5 = $r->exists('key1', 'key2', 'nosuchkey');
- Only one test generated, but there are 5 command results.
- However, easily patched up and works OK afterwards.

go-redis:
- Code looks OK, but the // STEP_START is before the start of the test function, while the // STEP_END is inside the body of the function. Ideally, the start/end markers should be just around the relevant snippet.

Jedis: looks fine.

Lettuce-async:
- Wrong assertThat is imported. It should be import static org.assertj.core.api.Assertions.assertThat;
- Has import and connect example steps (harmless, but I guess not needed for the example?)
- Unused import java.util.*; import. (again, harmless, but not needed)
- Otherwise looks fine.

Lettuce-reactive:
- Wrong assertThat is imported, as with async. It should be import static org.assertj.core.api.Assertions.assertThat;
- Unused import java.util.*; import.
- Otherwise looks fine.

@dwdougherty
Copy link
Collaborator Author

Thanks again, @andy-stark-redis. I've patched up those issues.

@dwdougherty dwdougherty requested a review from a team September 12, 2025 14:42
Copy link
Contributor

@andy-stark-redis andy-stark-redis left a comment

Choose a reason for hiding this comment

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

LGTM. I've approved but I guess we should add PRs for these to the respective client repos and see what the CI says?

@dwdougherty
Copy link
Collaborator Author

I have 11 more commands to do in this quarter's batch, and I thought I'd submit them to the client repos en masse. That said, I'm flexible.

@andy-stark-redis
Copy link
Contributor

@dwdougherty Batch is fine by me :-)

@dwdougherty dwdougherty merged commit 664eab5 into main Sep 12, 2025
5 checks passed
@dwdougherty dwdougherty deleted the DOC-5630 branch September 12, 2025 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants