Fix line number 0 unaligned display #142
Conversation
|
|
||
| // for issue 57 | ||
| #[test] | ||
| fn test_line_number_0() { |
There was a problem hiding this comment.
Could you change up the commits so this test case is the first commit, with it passing ie showing the bad behavior?
Benefits
- For someone viewing the commit with the behavior change, it makes it obvious what the intended behavior change is
- It shows to you and reviewers that the test tests the intended case
There was a problem hiding this comment.
For someone viewing the commit with the behavior change, it makes it obvious what the intended behavior change is
This means the commit that changes behavior should also include test updates. Every commit should pass tests
There was a problem hiding this comment.
As reminder, every commit should pass tests
There was a problem hiding this comment.
ae4694f is said to be a fix and changes behavior but there is no test update in that commit.
There was a problem hiding this comment.
Sorry I did not see the comment and please check out the update. Thanks.
|
BTW, I was thinking to add unit tests for eg. the |
|
FYI |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@epage |
For myself, I don't think its needed. btw I've not been looking too deeply at this until the commits get cleaned up |
chengr4
force-pushed
the
fix-unalign
branch
2 times, most recently
from
11f6daf to
f758ef6
Compare
Please clean up the commits to be how you want them reviewed and merged. I would recommend
|
|
I feel it is a little difficult to put refactor commits in advance. So I I put them at the end instead. |
chengr4
force-pushed
the
fix-unalign
branch
2 times, most recently
from
5a05078 to
1536c70
Compare
|
@epage I know this PR isn't a very necessary fix, but I'm curious if we can continue with it or if you'd prefer to close it? |
|
Looks like the commits aren't atomic. The "fix" commit doesn't include any test changes. Note when i asked for a test commit, I asked that it show the existing behavior (it should pass) and that the fix commit then makes the test to work with the fix. This makes it very clear what the change in behavior is. |
Running tests/formatter.rs (target/debug/deps/formatter-d39ddd6093209828)
running 40 tests
...
failures:
---- test_line_number_0 stdout ----
thread 'test_line_number_0' panicked at tests/formatter.rs:928:5:
---- expected: tests/formatter.rs:919:20
++++ actual: In-memory
1 1 | error: dummy
2 - --> file/path:0:3
3 - |
4 - 0 | foo
5 - | ^
6 - |∅
2 + --> file/path:0:3
3 + |
4 + 0 |foo
5 + | ^
6 + |∅
failures:
test_line_number_0
test result: FAILED. 39 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
- Line number width is 1 if max line number is 0 - Line number witdh is 0 if max line number is None - Update test_line_number_0 in tests/formatter.rs to reflect new behavior - Fix issue rust-lang#57 Running tests/formatter.rs (target/debug/deps/formatter-d39ddd6093209828) running 40 tests ... test result: ok. 40 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
The fold chaining is to find max line number rather than line number width
|
@epage I was thinking that these "test results" changes might be the 'test updates' you were hoping to see. If it is still incorrect, please guide me again. Thank you 🥹 |
fix #57