Skip to content

chore(ggplot2): follow-up Copilot review fixes after #6944 merge#6947

Merged
MarkusNeusinger merged 1 commit into
mainfrom
chore/ggplot2-followup
May 16, 2026
Merged

chore(ggplot2): follow-up Copilot review fixes after #6944 merge#6947
MarkusNeusinger merged 1 commit into
mainfrom
chore/ggplot2-followup

Conversation

@MarkusNeusinger
Copy link
Copy Markdown
Owner

Summary

Three Copilot review comments arrived on #6944 just before/after it was merged. Picking up the actionable ones in a small follow-up.

What's fixed

1. PlotOfTheDayTerminal — hardcoded python prompt (#discussion_r3253434308)
The terminal-style chip in the POTD card had a hardcoded python label even though the filename now flips to .R for ggplot2. Output would read python plots/<spec>/ggplot2.R — wrong. Now derives a runner token from potd.language (Rscript for r, python otherwise) alongside the existing ext switch.

2. prompts/plot-generator.md — contradictory docstring rule (#discussion_r3253434312)
The "Forbidden (Python)" list said "Type hints or docstrings (keep it simple)" while the same prompt elsewhere requires a 4-line module-header docstring at the top of every Python impl. Contradictory guidance can make generators omit the required header. Clarified: header docstring is mandatory, additional docstrings are forbidden. Added the R equivalent for roxygen #' blocks so the rule symmetric.

3. impl-review.yml — Python header rewrite needs a prepend fallback (#discussion_r3253434319)
The Python branch of the header-rewrite step ran re.sub on a triple-quoted docstring pattern and silently left the file unchanged if there was no header. Mirrors the R branch behaviour now: re.match first, prepend the canonical header if absent.

Deliberately not in this PR

  • SHA-pinning r-lib/actions/setup-r and setup-renv (#discussion_r3253434287, #discussion_r3253434297). Pinning is correct policy, but pushing a SHA I haven't verified would re-introduce the bogus-SHA blocker that feat(ggplot2): add R/ggplot2 library + multi-language pipeline #6944 already had to fix. Dependabot will pin them to a verified SHA on its next run, the same way every other action ref in the repo got there.

Test plan

  • python3 -c "import yaml; ..." on impl-review.yml — parses
  • CI: ci-lint, ci-tests, frontend tsc + vitest
  • One real impl-review run that exercises the Python prepend fallback (will happen organically the next time a Python impl is reviewed)

https://claude.ai/code/session_01Kb7b7QZi3ohtSTbd39poFV


Generated by Claude Code

Three Copilot review comments on PR #6944 that arrived after the merge:

1. PlotOfTheDayTerminal (#discussion_r3253434308)
   The terminal-style chip in the POTD card had a hardcoded "python" prompt
   even though the filename already flipped to .R for ggplot2. Result would
   read "python plots/<spec>/ggplot2.R" which is wrong. Now derives a
   `runner` token from `potd.language` (Rscript for r, python otherwise)
   alongside the existing ext switch.

2. prompts/plot-generator.md (#discussion_r3253434312)
   The "Forbidden (Python)" list said "Type hints or docstrings (keep it
   simple)" while the same prompt elsewhere requires a 4-line module-header
   docstring at the top of every Python impl. Contradictory guidance can
   make generators omit the required header. Clarified: header docstring is
   mandatory, additional docstrings (function/class level) are forbidden.
   Added the R equivalent of the rule for roxygen #' blocks.

3. .github/workflows/impl-review.yml (#discussion_r3253434319)
   The Python branch of the header-rewrite step ran re.sub on a triple-
   quoted docstring pattern but had no fallback if the impl file lacked a
   header — silently leaving the file without anyplot metadata. Mirrors the
   R branch behaviour: re.match first, prepend if no header is present.

Out of scope for this follow-up (still tracked):
- SHA-pinning r-lib/actions/setup-r and setup-renv (would need a verified
  upstream SHA; let Dependabot pin them on its next run, consistent with
  how the rest of the repo's action refs landed).
Copilot AI review requested due to automatic review settings May 16, 2026 20:48
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Small follow-up to PR #6944 (R/ggplot2 support) addressing three Copilot review comments: fix a hardcoded python label in the POTD terminal chip, clarify contradictory docstring guidance in the plot-generator prompt, and add a prepend fallback to the Python header-rewrite step in impl-review.

Changes:

  • POTD terminal now derives runner (Rscript vs python) from potd.language, matching the existing .R/.py extension switch.
  • Plot-generator prompt clarifies that the 4-line module header is mandatory while extra docstrings/roxygen blocks are forbidden (Python + R).
  • impl-review.yml Python header rewrite uses re.match first and prepends the canonical header if absent, mirroring the R branch.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
app/src/components/PlotOfTheDayTerminal.tsx Derive runner from language; replace hardcoded python chip text.
prompts/plot-generator.md Resolve contradiction: mandatory header docstring, no extra docstrings/roxygen.
.github/workflows/impl-review.yml Add prepend fallback when Python impl has no leading triple-quoted docstring.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@MarkusNeusinger MarkusNeusinger merged commit e300005 into main May 16, 2026
13 checks passed
@MarkusNeusinger MarkusNeusinger deleted the chore/ggplot2-followup branch May 16, 2026 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants