Dspx 2178 claudit #364
Dspx 2178 claudit #364
Conversation
this will hopefully reduce context overhead while refactoring
vibe-coded (sonnet 4.5) utility script for quickly getting a peek at what is currently available to test in xtest
Summary of ChangesHello @dmihalcik-virtru, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the xtest test suite. The primary change involves moving pytest fixtures from a large conftest.py file into a more modular and maintainable fixtures package, organized by domain (KAS, attributes, keys, etc.). Additionally, the OpentdfCommandLineTool has been extracted into its own otdfctl.py module, improving separation of concerns. These changes greatly enhance the structure and readability of the test code. The addition of documentation for AI agents is also a nice touch. My review includes a couple of minor suggestions for improvement, but overall, this is a high-quality contribution.
| from otdfctl import OpentdfCommandLineTool | ||
|
|
||
|
|
||
| def create_temp_namesapce(otdfctl: OpentdfCommandLineTool): |
There was a problem hiding this comment.
There's a typo in the function name create_temp_namesapce. It should be create_temp_namespace. Please correct the function name here and at its call sites within this file (lines 30, 397, and 440) for consistency and to prevent confusion.
| def create_temp_namesapce(otdfctl: OpentdfCommandLineTool): | |
| def create_temp_namespace(otdfctl: OpentdfCommandLineTool): |
| # Parse JSON and output as table | ||
| echo "SDK VERSION SHA BUILT PATH" | ||
| echo "───── ──────────────── ──────── ───── ────────────────────────────────────────" | ||
|
|
||
| for sdk in go java js; do | ||
| # Extract worktrees for this SDK | ||
| worktrees=$(echo "$JSON_OUTPUT" | grep -A 999 "\"$sdk\":" | grep -A 999 "\"worktrees\":" | sed -n '/\[/,/\]/p' | grep -v "bare_repo") | ||
|
|
||
| # Parse each worktree | ||
| while IFS= read -r line; do | ||
| if [[ "$line" =~ \"version\":[[:space:]]*\"([^\"]+)\" ]]; then | ||
| version="${BASH_REMATCH[1]}" | ||
| fi | ||
| if [[ "$line" =~ \"sha\":[[:space:]]*\"([^\"]+)\" ]]; then | ||
| sha="${BASH_REMATCH[1]}" | ||
| sha_short="${sha:0:7}" | ||
| fi | ||
| if [[ "$line" =~ \"built\":[[:space:]]*(true|false) ]]; then | ||
| built="${BASH_REMATCH[1]}" | ||
| if [[ "$built" == "true" ]]; then | ||
| built_symbol="✓" | ||
| else | ||
| built_symbol="✗" | ||
| fi | ||
| fi | ||
| if [[ "$line" =~ \"path\":[[:space:]]*\"([^\"]+)\" ]]; then | ||
| path="${BASH_REMATCH[1]}" | ||
| # Print the row when we have all data | ||
| if [[ -n "${version:-}" && -n "${sha_short:-}" && -n "${built_symbol:-}" && -n "${path:-}" ]]; then | ||
| printf "%-6s %-17s %-9s %-6s %s\n" "$sdk" "$version" "$sha_short" "$built_symbol" "$path" | ||
| version="" | ||
| sha_short="" | ||
| built_symbol="" | ||
| path="" | ||
| fi | ||
| fi | ||
| done <<< "$worktrees" | ||
| done |
There was a problem hiding this comment.
The current implementation for parsing JSON to create the table format is complex and potentially fragile, as it relies on grep, sed, and bash regex matching. For improved robustness and maintainability, consider using a dedicated JSON processor like jq. It would simplify the parsing logic significantly.
Here's an example of how you could generate the table rows using jq:
echo "$JSON_OUTPUT" | jq -r 'to_entries | .[] | .key as $sdk | .value.worktrees[]? | [$sdk, .version, (.sha | .[0:7]), (if .built then "✓" else "✗" end), .path] | @tsv' | while IFS=$'\t' read -r sdk version sha_short built_symbol path; do printf "%-6s %-17s %-9s %-6s %s\n" "$sdk" "$version" "$sha_short" "$built_symbol" "$path"; doneThis approach would be more resilient to changes in the JSON structure or formatting. If jq is not a desirable dependency, this implementation is acceptable, but jq is highly recommended for shell scripting involving JSON.
1 participant
No description provided.