It's a good practice to start your bash scripts withset -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.

scripts/whats_left_modules.sh does not include this declaration.

The current implementation for grouping items is complex and fragile. It relies on the input data being sorted and will produce invalid HTML if thebuiltin_items list is empty. You can significantly simplify this logic and make it more robust by using Liquid'sgroup_by filter. This would also remove the need for theprettier workaround comment.

  <div>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><ol>    {% for item in group.items %}      <li><code>{{ item.name }}</code> {% if item.is_inherited %}{{ item.is_inherited }}{% endif %}</li>    {% endfor %}    </ol></div>  {% endfor %}

@gemini-code-assist I have pushed a commit to address this.

That's great to hear! I've reviewed the updated code in_layouts/whats_left.html, and it looks like you've successfully implemented thegroup_by filter as suggested. This significantly improves the clarity and robustness of the item grouping logic. Well done!

The combination ofecho,sed, andawk with redirection can be simplified into a single, more readableawk script. This improves maintainability by making the logic for parsing the temp file and generating the CSV self-contained and easier to understand.

@gemini-code-assist Is it possible to rework the suggestion to use a heredoc?

@gemini-code-assist Is it possible to rework the suggestion to use a heredoc?

Yes, it's definitely possible to rework the suggestion to use a heredoc. This can make theawk script more self-contained and potentially easier to read within the shell script. Here's an updated suggestion that uses a heredoc for theawk script:

This approach replaces theecho command for the header and thesed | awk pipeline with a singleawk command that reads its script from the heredoc. Theawk script itself handles printing the header and processing the relevant section of the temp file.

There's a minor formatting issue here that results in an extraneous trailing space whenitem.is_inherited is not present. This happens because the space before theif statement is unconditional. Moving the space inside the conditional block will ensure it only appears when needed.

          <li><code>{{ item.name }}</code>{% if item.is_inherited %} {{ item.is_inherited }}{% endif %}</li>

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 thescripts 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/bashset -euo pipefailSCRIPT_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 existmkdir -p"$OUTPUT_DIR"# exit violently if the temp file does not existif [!-f"$TEMP_FILE" ];thenecho"error: input file$TEMP_FILE not found">&2exit 1fi# generate the CSV file for builtin items from the temp fileawk -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

The current method of extracting theis_inherited part of the line is not robust against multiple spaces between the item name and the tag. For example, if a line wereitem (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.

@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

You are absolutely right,@fanninpm. My apologies for overlooking the specific guarantee provided by the Python script atwhats_left.py#L477. Given thatinherited_str is either an empty string or " (inherited)", the input to theawk script will indeed have at most one space before(inherited). In this scenario, the currentawk logicrest = substr($0, idx+1) correctly extracts(inherited) without any leading spaces, making my previous suggestion unnecessary. Thank you for clarifying the input format!

#!/bin/bash# Fetch the referenced line from the upstream RustPython repositorycurl -s "https://raw.githubusercontent.com/RustPython/RustPython/5cad66cebfc62a7912197ae299ac13e8629c560d/whats_left.py" | sed -n '470,485p'

@fanninpm, you're absolutely right! I can confirm from the upstream code that line 477 uses:

print(f"{module}.{method}"+ (f"{reason}"ifreasonelse""))

This guarantees the format is eithermodule.method ormodule.method {reason} with exactly one space before the reason when present. Your current approach of usingsubstr($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.