Add builtin_items data to whats_left.html #81
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a "Built-in Items" section: a new script extracts the "# builtin items" block from Changes
Sequence Diagram(s)sequenceDiagram
participant Temp as _data/whats_left.temp
participant Script as scripts/whats_left_builtin_items.sh
participant CSV as _data/whats_left/builtin_items.csv
participant Layout as _layouts/whats_left.html
participant Output as Generated HTML
note over Temp,Script: Extract the "# builtin items" block (stop at blank line)
Script->>Temp: read and isolate block
Script->>Script: parse lines (split first field on ".", capture rest after first space)
Script->>CSV: write header and append CSV rows
Layout->>CSV: read CSV and group rows by `builtin`
loop per builtin group
Layout->>Layout: render group header (`h4`) and open two-column container
Layout->>Output: render items as `code` name + optional `is_inherited`
Layout->>Layout: close group container
end
Layout->>Output: finalize layout
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
_layouts/whats_left.html (1)
46-46: Clarify the prettier workaround comment.The commented HTML on line 46 is non-standard and should be documented. Consider either:
- Adding an inline comment explaining why this specific HTML structure prevents prettier from crashing, or
- Linking to a GitHub issue that explains the workaround.
This will help future maintainers understand the constraint without removing or refactoring the code.
scripts/whats_left_builtin_items.sh (2)
11-12: Clarify the awk data transformation logic and input format.The awk command assumes a specific input format from the temp file, but this isn't documented. The current logic:
- Splits
$1on dots to extract the builtin part (everything before first dot)- Uses everything after the first field separator as the
is_inheritedindicatorThis works if the input format is:
builtin.name [inherited_marker], but the logic should be clearer. Consider adding a comment above the sed/awk pipeline documenting the expected input format from../_data/whats_left.temp.Example:
# Expected format of builtin items in whats_left.temp: # builtin.item_name [optional_inherited_indicator] sed -n '/# builtin items/{n;:a;/^$/q;p;n;ba;}' ../_data/whats_left.temp | \ awk -v OFS=',' '{split($1,a,".");if(index($0,FS)>0){b=substr($0,index($0,FS)+1)}else{b=""};print a[1],$1,b}' >> ../_data/whats_left/builtin_items.csv
1-12: Add error handling for robustness.The script lacks error handling for common failure cases:
- The temp file
../_data/whats_left.tempmay not exist- The sed/awk pipeline could fail silently
- No verification that the CSV was successfully generated
Consider adding checks:
#!/bin/bash + set -e # Exit on any error # create directory if it doesn't exist mkdir -p ../_data/whats_left + # Verify temp file exists + if [[ ! -f ../_data/whats_left.temp ]]; then + echo "Error: ../_data/whats_left.temp not found" >&2 + exit 1 + fi # start a new csv file for builtin items echo "builtin,name,is_inherited" > ../_data/whats_left/builtin_items.csvThis ensures the script fails visibly if preconditions aren't met.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
_data/whats_left/builtin_items.csvis excluded by!**/*.csv
📒 Files selected for processing (2)
_layouts/whats_left.html(1 hunks)scripts/whats_left_builtin_items.sh(1 hunks)
🔇 Additional comments (1)
_layouts/whats_left.html (1)
44-58: Built-in Items section structure is sound.The grouping logic using
previous_builtincorrectly closes and reopens sections when the builtin category changes. The conditional on line 48 ensures no closing tags are output before the first group, and line 58 properly closes the final list structure.
Summary of ChangesHello @fanninpm, 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 introduces a new feature to enhance the Highlights
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://waps.l3s.uni-hannover.de/live/im_/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 new section to the whats_left.html page, displaying unimplemented built-in items from a newly generated CSV file. The changes are functional, but I've identified opportunities to improve the robustness and maintainability of both the new shell script and the HTML template. My suggestions include making the shell script safer with set -euo pipefail, refactoring a complex sed | awk pipeline into a single, more readable awk command, and using Liquid's group_by filter in the HTML to simplify the logic and fix potential bugs.
| @@ -0,0 +1,12 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
It's a good practice to start your bash scripts with set -euo pipefail. This makes the script more robust by exiting on errors, treating unset variables as errors, and ensuring that a pipeline's exit status is the exit status of the last command to exit with a non-zero status.
| #!/bin/bash | |
| #!/bin/bash | |
| set -euo pipefail |
There was a problem hiding this comment.
scripts/whats_left_modules.sh does not include this declaration.
_layouts/whats_left.html
Outdated
| <div class="section-title">What's left: Built-in Items</div> | ||
| {% assign previous_builtin = "" %} | ||
| {% comment %}<div><ol><!-- this makes prettier not crash -->{% endcomment %} | ||
| {% for item in site.data.whats_left.builtin_items %} | ||
| {% if item.builtin != previous_builtin %} | ||
| {% if previous_builtin != "" %} | ||
| </ol></div> | ||
| {% endif %} | ||
| <h4>{{ item.builtin }}</h4> | ||
| <div class="column-count-is-2"><ol> | ||
| {% endif %} | ||
| <li class="word-wrap-anywhere"><code>{{ item.name }}</code> {% if item.is_inherited %}{{ item.is_inherited }}{% endif %}</li> | ||
| {% assign previous_builtin = item.builtin %} | ||
| {% endfor %} | ||
| </ol></div> |
There was a problem hiding this comment.
The current implementation for grouping items is complex and fragile. It relies on the input data being sorted and will produce invalid HTML if the builtin_items list is empty. You can significantly simplify this logic and make it more robust by using Liquid's group_by filter. This would also remove the need for the prettier workaround comment.
<div class="section-title">What's left: Built-in Items</div>
{% assign items_by_builtin = site.data.whats_left.builtin_items | group_by: "builtin" %}
{% for group in items_by_builtin %}
<h4>{{ group.name }}</h4>
<div class="column-count-is-2"><ol>
{% for item in group.items %}
<li class="word-wrap-anywhere"><code>{{ item.name }}</code> {% if item.is_inherited %}{{ item.is_inherited }}{% endif %}</li>
{% endfor %}
</ol></div>
{% endfor %}
There was a problem hiding this comment.
@gemini-code-assist I have pushed a commit to address this.
There was a problem hiding this comment.
That's great to hear! I've reviewed the updated code in _layouts/whats_left.html, and it looks like you've successfully implemented the group_by filter as suggested. This significantly improves the clarity and robustness of the item grouping logic. Well done!
scripts/whats_left_builtin_items.sh
Outdated
| # start a new csv file for builtin items | ||
| echo "builtin,name,is_inherited" > ../_data/whats_left/builtin_items.csv | ||
|
|
||
| # read the temp file | ||
| # in awk: | ||
| sed -n '/# builtin items/{n;:a;/^$/q;p;n;ba;}' ../_data/whats_left.temp | \ | ||
| awk -v OFS=',' '{split($1,a,".");if(index($0,FS)>0){b=substr($0,index($0,FS)+1)}else{b=""};print a[1],$1,b}' >> ../_data/whats_left/builtin_items.csv |
There was a problem hiding this comment.
The combination of echo, sed, and awk with redirection can be simplified into a single, more readable awk script. This improves maintainability by making the logic for parsing the temp file and generating the CSV self-contained and easier to understand.
| # start a new csv file for builtin items | |
| echo "builtin,name,is_inherited" > ../_data/whats_left/builtin_items.csv | |
| # read the temp file | |
| # in awk: | |
| sed -n '/# builtin items/{n;:a;/^$/q;p;n;ba;}' ../_data/whats_left.temp | \ | |
| awk -v OFS=',' '{split($1,a,".");if(index($0,FS)>0){b=substr($0,index($0,FS)+1)}else{b=""};print a[1],$1,b}' >> ../_data/whats_left/builtin_items.csv | |
| # Generate the CSV file for builtin items from the temp file | |
| awk ' | |
| BEGIN { | |
| OFS="," | |
| print "builtin,name,is_inherited" | |
| } | |
| /^# builtin items$/ { in_section=1; next } | |
| /^$/ { if(in_section) exit } | |
| in_section { | |
| split($1, a, ".") | |
| rest = "" | |
| idx = index($0, " ") | |
| if (idx > 0) { | |
| rest = substr($0, idx + 1) | |
| } | |
| print a[1], $1, rest | |
| } | |
| ' ../_data/whats_left.temp > ../_data/whats_left/builtin_items.csv |
There was a problem hiding this comment.
@gemini-code-assist Is it possible to rework the suggestion to use a heredoc?
There was a problem hiding this comment.
@gemini-code-assist Is it possible to rework the suggestion to use a heredoc?
There was a problem hiding this comment.
Yes, it's definitely possible to rework the suggestion to use a heredoc. This can make the awk script more self-contained and potentially easier to read within the shell script. Here's an updated suggestion that uses a heredoc for the awk script:
This approach replaces the echo command for the header and the sed | awk pipeline with a single awk command that reads its script from the heredoc. The awk script itself handles printing the header and processing the relevant section of the temp file.
| # start a new csv file for builtin items | |
| echo "builtin,name,is_inherited" > ../_data/whats_left/builtin_items.csv | |
| # read the temp file | |
| # in awk: | |
| sed -n '/# builtin items/{n;:a;/^$/q;p;n;ba;}' ../_data/whats_left.temp | \ | |
| awk -v OFS=',' '{split($1,a,".");if(index($0,FS)>0){b=substr($0,index($0,FS)+1)}else{b=""};print a[1],$1,b}' >> ../_data/whats_left/builtin_items.csv | |
| # Generate the CSV file for builtin items from the temp file | |
| awk -f - ../_data/whats_left.temp > ../_data/whats_left/builtin_items.csv << 'EOF' | |
| BEGIN { | |
| OFS="," | |
| print "builtin,name,is_inherited" | |
| } | |
| /^# builtin items$/ { in_section=1; next } | |
| /^$/ { if(in_section) exit } | |
| in_section { | |
| split($1, a, ".") | |
| rest = "" | |
| idx = index($0, " ") | |
| if (idx > 0) { | |
| rest = substr($0, idx + 1) | |
| } | |
| print a[1], $1, rest | |
| } | |
| EOF |
This obviates the need for the Prettier workaround.
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
👍 please merge it when you finished revision
|
@gemini-code-assist please re-review. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Built-in Items' section to the whats_left.html page, complete with a data generation script. The implementation is solid, and the new section is a great addition. I've included a couple of minor suggestions to enhance the script's user-friendliness and to correct a small formatting detail in the HTML output.
_layouts/whats_left.html
Outdated
| <div class="column-count-is-2"> | ||
| <ol> | ||
| {% for item in group.items %} | ||
| <li class="word-wrap-anywhere"><code>{{ item.name }}</code> {% if item.is_inherited %}{{ item.is_inherited }}{% endif %}</li> |
There was a problem hiding this comment.
There's a minor formatting issue here that results in an extraneous trailing space when item.is_inherited is not present. This happens because the space before the if statement is unconditional. Moving the space inside the conditional block will ensure it only appears when needed.
<li class="word-wrap-anywhere"><code>{{ item.name }}</code>{% if item.is_inherited %} {{ item.is_inherited }}{% endif %}</li>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
@gemini-code-assist please re-review. |
There was a problem hiding this comment.
Code Review
This pull request adds a new section for 'Built-in Items' to the 'What's Left' page. This is achieved by adding a script to generate a CSV data file from a temporary file, and then updating the layout to display this data, grouped by built-in type. The changes look good and the implementation is straightforward. I've suggested one improvement to the new shell script to make it more robust by not relying on the current working directory. Otherwise, the changes are solid.
| #!/bin/bash | ||
| set -euo pipefail | ||
|
|
||
| # create directory if it doesn't exist | ||
| mkdir -p ../_data/whats_left | ||
|
|
||
| # exit violently if the temp file does not exist | ||
| if [ ! -f ../_data/whats_left.temp ]; then | ||
| echo "error: input file ../_data/whats_left.temp not found" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| # generate the CSV file for builtin items from the temp file | ||
| awk -f - ../_data/whats_left.temp > ../_data/whats_left/builtin_items.csv <<'EOF' | ||
| BEGIN { | ||
| OFS="," | ||
| print "builtin,name,is_inherited" | ||
| } | ||
| /^# builtin items/ { in_section=1; next } | ||
| /^$/ { if (in_section) exit } | ||
| in_section { | ||
| split($1, a, ".") | ||
| rest = "" | ||
| idx = index($0, " ") | ||
| if (idx > 0) { | ||
| rest = substr($0, idx+1) | ||
| } | ||
| print a[1], $1, rest | ||
| } | ||
| EOF |
There was a problem hiding this comment.
This script uses relative paths (e.g., ../_data), which makes its execution dependent on the current working directory. It will fail if not run from a directory where ../_data resolves correctly (like the scripts directory or the repository root). To make it more robust and runnable from any location, it's a best practice in shell scripting to define paths relative to the script's own location.
#!/bin/bash
set -euo pipefail
SCRIPT_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
PROJECT_ROOT=$(cd "$SCRIPT_DIR/.." && pwd)
DATA_DIR="$PROJECT_ROOT/_data"
TEMP_FILE="$DATA_DIR/whats_left.temp"
OUTPUT_DIR="$DATA_DIR/whats_left"
OUTPUT_FILE="$OUTPUT_DIR/builtin_items.csv"
# create directory if it doesn't exist
mkdir -p "$OUTPUT_DIR"
# exit violently if the temp file does not exist
if [ ! -f "$TEMP_FILE" ]; then
echo "error: input file $TEMP_FILE not found" >&2
exit 1
fi
# generate the CSV file for builtin items from the temp file
awk -f - "$TEMP_FILE" > "$OUTPUT_FILE" <<'EOF'
BEGIN {
OFS=","
print "builtin,name,is_inherited"
}
/^# builtin items/ { in_section=1; next }
/^$/ { if (in_section) exit }
in_section {
split($1, a, ".")
rest = ""
idx = index($0, " ")
if (idx > 0) {
rest = substr($0, idx+1)
}
print a[1], $1, rest
}
EOF
|
@gemini-code-assist please re-review. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Built-in Items' section to the 'whats_left.html' page, complete with a data generation script and HTML rendering logic. The changes are well-implemented. The new shell script correctly parses the temporary file to generate a CSV, and the Liquid templating in the HTML file effectively groups and displays this data. I have one minor suggestion for the shell script to improve its robustness when parsing the data, which will enhance data quality and maintainability.
| rest = "" | ||
| idx = index($0, " ") | ||
| if (idx > 0) { | ||
| rest = substr($0, idx+1) |
There was a problem hiding this comment.
The current method of extracting the is_inherited part of the line is not robust against multiple spaces between the item name and the tag. For example, if a line were item (inherited), the value in the CSV would be (inherited) with a leading space. This can lead to inconsistent data.
A more robust approach is to take the substring from the first space onwards and then remove any leading spaces from it.
| rest = substr($0, idx+1) | |
| rest = substr($0, idx) | |
| sub(/^ +/, "", rest) |
There was a problem hiding this comment.
@gemini-code-assist @coderabbitai The approach is dependent on the behavior of this line, which currently guarantees one period and at most one space: https://github.com/RustPython/RustPython/blob/5cad66cebfc62a7912197ae299ac13e8629c560d/whats_left.py#L477
There was a problem hiding this comment.
You are absolutely right, @fanninpm. My apologies for overlooking the specific guarantee provided by the Python script at whats_left.py#L477. Given that inherited_str is either an empty string or " (inherited)", the input to the awk script will indeed have at most one space before (inherited). In this scenario, the current awk logic rest = substr($0, idx+1) correctly extracts (inherited) without any leading spaces, making my previous suggestion unnecessary. Thank you for clarifying the input format!
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the referenced line from the upstream RustPython repository
curl -s "https://raw.githubusercontent.com/RustPython/RustPython/5cad66cebfc62a7912197ae299ac13e8629c560d/whats_left.py" | sed -n '470,485p'
Length of output: 539
@fanninpm, you're absolutely right! I can confirm from the upstream code that line 477 uses:
print(f"{module}.{method}" + (f" {reason}" if reason else ""))This guarantees the format is either module.method or module.method {reason} with exactly one space before the reason when present. Your current approach of using substr($0, idx+1) correctly extracts just the reason text by skipping that single guaranteed space.
Given this upstream format guarantee, the defensive handling of multiple spaces isn't necessary. Your implementation is appropriate and correctly relies on the documented behavior from the source.
2 participants
I had to do an ugly thing to keep prettier from crashing.
Summary by CodeRabbit
New Features
Chores