Conversation
![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 implements dynamic logo and color extraction for Stern Pinball apps by scraping metadata from their web service. The review identified a critical bug in stern_high_scores.star where score filtering is bypassed. Feedback also highlighted brittle string parsing logic for hex colors and URLs, recommending more robust delimiter detection over magic numbers and fixed-length slicing. Suggestions were provided to optimize performance by pre-calculating colors and to adhere to snake_case naming conventions.
| c1 = games_body[hex_start:hex_start+7] | ||
|
|
||
| g2_idx = games_body.find('gradient_stop', idx, idx + 5000) | ||
| if g2_idx > -1: | ||
| hex_start = games_body.find('#', g2_idx, g2_idx + 50) | ||
| if hex_start > -1: | ||
| c2 = games_body[hex_start:hex_start+7] |
There was a problem hiding this comment.
Extracting a fixed 7 characters for hex colors (lines 116 and 122) is brittle. If the source string uses 3-digit hex codes or if the field is followed immediately by a quote or backslash, color.hex() will receive an invalid string and fail. It is safer to find the actual end of the value by searching for the closing quote.
| # Build personal score table | ||
| score_rows = [] | ||
| for s in scores: | ||
| for s in g["scores"]: |
There was a problem hiding this comment.
The loop now iterates over g["scores"] instead of the local scores variable. This causes the highest_only filter (applied on lines 220-225) to be ignored, as it modifies the local scores list but not the one inside the g dictionary.
| for s in g["scores"]: | |
| for s in scores: |
| if end1 == -1: end1 = 999999 | ||
| if end2 == -1: end2 = 999999 | ||
| s_end = end1 if end1 < end2 else end2 |
There was a problem hiding this comment.
The use of a magic number like 999999 to handle missing delimiters is fragile. A more robust approach would be to check if the indices are valid (not -1) and use the minimum of the valid ones.
| "c1": c1, | ||
| "c2": c2, |
There was a problem hiding this comment.
Pre-calculate the middle color here to avoid redundant calls to mid_color inside the rendering loop in main.
"c1": c1,
"c2": c2,
"mid_c": mid_color(c1, c2),
References
- Local variables should use snake_case (e.g., mid_c) to distinguish them from global constants, following PEP 8 conventions. (link)
| if s["model"] == "pro": | ||
| model_color = g["c1"] | ||
| elif s["model"] == "premium": | ||
| model_color = mid_color(g["c1"], g["c2"]) |
There was a problem hiding this comment.
Use the pre-calculated middle color instead of calling the function repeatedly.
| model_color = mid_color(g["c1"], g["c2"]) | |
| model_color = g.get("mid_c", "#666") |
References
- Local variables should use snake_case (e.g., model_color) to distinguish them from global constants, following PEP 8 conventions. (link)
| end1 = games_body.find('\\"', http_start) | ||
| end2 = games_body.find('"', http_start) | ||
| if end1 == -1: end1 = 999999 | ||
| if end2 == -1: end2 = 999999 | ||
| s_end = end1 if end1 < end2 else end2 |
There was a problem hiding this comment.
The logic for finding the end of the URL using a magic number 999999 is fragile. Additionally, this parsing logic is duplicated from stern_high_scores.star. Consider a more robust way to detect the closing delimiter for the URL.
1 participant
New API old logos