Internal workflow to see docs impact from committed modifications#910
Internal workflow to see docs impact from committed modifications#910LasmarKhalifa wants to merge 3 commits into
Conversation
LasmarKhalifa
force-pushed
the
05-26/branch-docs-impact-workflow
branch
from
865467a to
0250540
Compare
LasmarKhalifa
changed the title
LasmarKhalifa
changed the title
LasmarKhalifa
requested review from
dersam,
juniper-shopify,
mathiusj and
nfgrep
as code owners
dersam
left a comment
dersam
left a comment
There was a problem hiding this comment.
I don't see anything specific to Roast in this workflow. Can we add it to examples and provide functional tests for it so anyone can reuse it?
juniper-shopify
left a comment
juniper-shopify
left a comment
There was a problem hiding this comment.
A bunch of suggestions for enhancements, but this is good to go in my opinion as a first phase
|
|
||
| #: self as Roast::Workflow | ||
|
|
||
| # Gets the committed diff of the current branch vs origin/main, then analyzes it for potential |
There was a problem hiding this comment.
Consider looking at the graphite branch's parent branch instead of always main
|
|
||
| agent(:analyzer) do | ||
| skip! if cmd!(:diff).text.strip.empty? | ||
| fail!("diff too large (#{cmd!(:diff).text.bytesize} bytes) to analyze — narrow the branch or exclude generated files") if cmd!(:diff).text.bytesize > 500_000 |
There was a problem hiding this comment.
nice edge case to check! Consider adding a force arg to bypass it? Or a max_diff_size kwarg to override the default and 0 is unlimited?
| Rules: | ||
| - Use ONLY the diff below. Do not run any commands. | ||
| - Be thorough about finding real issues — do not be conservative. | ||
| - But do NOT speculate about docs you cannot see in the diff. |
There was a problem hiding this comment.
Wouldn't it be good to have it look at the code diff vs all the docs? not just changed docs?
| skip! if agent!(:analyzer).response.strip == "No documentation impact." | ||
| <<~PROMPT | ||
| Apply the documentation fixes suggested in the analysis below. Edit the | ||
| affected files in place. Do not modify any code files; docs only. |
There was a problem hiding this comment.
We would want it to modify method and class doc comments in code files, though, right?
| Apply the documentation fixes suggested in the analysis below. Edit the | ||
| affected files in place. Do not modify any code files; docs only. | ||
|
|
||
| #{agent!(:analyzer).response} |
There was a problem hiding this comment.
Maybe just resume from the analyzer's session
do my.session = agent!(:analyzer.session) before the prompt
There was a problem hiding this comment.
You can still include the report in your prompt, potentially, to be extra precise about what you want, though
| puts "Files considered (#{files.size}):" | ||
| files.each { |f| puts " #{f}" } | ||
| puts "ANALYSIS:\n#{chat!(:report).response}" | ||
| puts(agent?(:fixer) ? "Fixes applied." : "(Next time, run with `-- fix` to auto-apply suggested edits)") |
There was a problem hiding this comment.
might be worth storing some report data/metadata so that when you run with fix right after a dry run it doesn't have to re-analyze
I'm not sure I'd want our internal maintenance workflows to live in the |
This is a good idea to do regardless. Workflows that we use for internal maintenance should ideally be tested I would do this as a follow-up PR after the full stack of PRs building this workflow out is merged, though. |
Makes sense to me. |
LasmarKhalifa
force-pushed
the
05-26/branch-docs-impact-workflow
branch
from
0250540 to
ee00173
Compare
3 participants
tldr; Adds a workflow that takes committed changes on a branch, and analyzes their impact on Roast's docs.
If the
-- fixargument is passed, automatic fixes will be applied.This was placed under internal/workflows/maintenance