Display line diffs in dsl --verify error messages#2592
Conversation
dejmedus
force-pushed
the
jb-add-diff-output-to-error-msg
branch
3 times, most recently
from
f3c5b51 to
94ecc33
Compare
| raise Tapioca::Error, <<~ERROR | ||
| file_diff = if diff_output.lines.count.between?(1, 250) | ||
| "#{set_color("Diff:", :red)}\n#{diff_output.chomp}" | ||
| end |
There was a problem hiding this comment.
Do you think it's useful to output a message when the diff is longer than 250 lines and explain we're omitting it.
| #{set_color("Reason:", :red)} | ||
| #{reasons} | ||
|
|
||
| #{file_diff} |
There was a problem hiding this comment.
We should prevent this from printing a blank line if it's nil. You could prepend \n\n to file_diff above and then do #{reasons}#{file_diff}.
Another solution is doing 2 chomp calls. It might be better, I'm not sure.
| end.join("\n") | ||
|
|
||
| raise Tapioca::Error, <<~ERROR | ||
| file_diff = if diff_output.lines.count.between?(1, 250) |
There was a problem hiding this comment.
Should this 250 limit be a CLI option instead and default to 250? dsl --verify --max-diff-lines=1000
|
|
||
| stdout | ||
| rescue => e | ||
| say_error("Failed to create #{filename} diff. #{e.message}", :yellow) |
There was a problem hiding this comment.
Why did you opt for yellow and not red? I think red is more consistent with rest of our errors.
There was a problem hiding this comment.
I looked through say_error uses and it seemed like :yellow was used for warnings and :red for errors. I leaned towards this being a warning. Makes sense, changed to :red :)
| Loading DSL extension classes... Done | ||
| Loading Rails application... Done | ||
| Loading DSL compiler classes... Done | ||
| Checking for out-of-date RBIs... |
There was a problem hiding this comment.
Did you make this change to be more consistent with other tests? I assume it's not needed.
There was a problem hiding this comment.
This was so I could assert stdout_equals rather than stdout_includes to ensure there isn't a stray Diff:
| @command = DslVerify.new( | ||
| requested_constants: [], | ||
| requested_paths: [], | ||
| outpath: Pathname.new("tmp"), |
There was a problem hiding this comment.
Does DslVerify write something to the file system? I think other DslVerify tests run on a temporary directory that's cleaned up but this is not an integration test so it shouldn't use that, it's more costly.
If it is writing something we should clean up after, maybe we can create @tmp = Dir.mktmpdir in the before block, use that as the outpath and then clean it up in an after block.
There was a problem hiding this comment.
If im understanding correctly, outpath is required to create DslVerify but the command isn't being executed here, I just need an instance to pull file_diff from. I changed it to /dev/null to make it clearer that its unused. tbh I wouldn't be that against removing this test file if we don't find it helpful, file diff is being tested by verify tests also
There was a problem hiding this comment.
Actually ignore above, I moved out file_diff to rbi_files_helpers and the tests look much nicer now :)
dejmedus
force-pushed
the
jb-add-diff-output-to-error-msg
branch
2 times, most recently
from
5e37dd0 to
0aa9400
Compare
When `dsl --verify` detects out of date RBI files, the error message lists which files were added, removed, or changed. We could make it easier to understand what has changed by including the line diff output in the error message as well This commit grabs the line diff output during RBI verification and includes it in the error message when the diff is 250 lines or less
dejmedus
force-pushed
the
jb-add-diff-output-to-error-msg
branch
from
0aa9400 to
6bdba8a
Compare
2 participants
Motivation
When
tapioca dsl --verifydetects out of date RBI files, the error message lists which files were added, removed, or changed. We could make it easier to visualize these changes by including the line diff output in the error messageOutput example
Implementation
This PR diffs updated files against the originals during RBI verification and then includes the output in the error message when the diff is 250 lines or less, or whatever value is provided to
--max-diff-linesflagTests
verifytests to include line diff