-
Notifications
You must be signed in to change notification settings - Fork 45
Fix line number 0 unaligned display #142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2d2d0e4 to
d445d69
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As reminder, every commit should pass tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I did not see the comment and please check out the update. Thanks.
d445d69 to
1180660
Compare
BTW, I was thinking to add unit tests for eg. the format_line method. it seemed quite challenging 😅, so I gave up. If anyone is willing to guide me on this, I would be very happy to give it another try.
FYI
In the last commit, I extracted the calculation to another method. I'm not sure if it is proper, but I'm trying to make fmt adhere more closely to the Single Responsibility Principle
43a1dc0 to
7a51b73
Compare
@epage
I thought about it and decided to refactor like this. If you feel the last commit is over-engineering, please let me know, and I'll delete it. Thanks.
I thought about it and decided to refactor like this. If you feel the last commit is over-engineering, please let me know, and I'll delete it. Thanks.
For myself, I don't think its needed.
btw I've not been looking too deeply at this until the commits get cleaned up
7a51b73 to
0c21810
Compare
11f6daf to
f758ef6
Compare
btw I've not been looking too deeply at this until the commits get cleaned up
Please clean up the commits to be how you want them reviewed and merged.
I would recommend
- refactor commits
- test demonstrating the problem
- fix with test update showing new behavior
f758ef6 to
ea22691
Compare
I feel it is a little difficult to put refactor commits in advance. So I I put them at the end instead.
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
1536c70 to
7863752
Compare
@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 🥹
@chengr4 Every commit should pass tests. That 7ffe4b2 has no test changes either means the fix does not actually fix the problem or that the previous commit does not pass tests. See clap-rs/clap#5520 as an example of this.
I would note that a major rewrite is in work, so I would recommend coordinating with @Muscraft on whether you should wait until after that happens before rebasing.
The renderer was changed 0.12 and this may not be a problem anymore but we should ensure we have tests covering this
Confirmed this isn't an issue anymore though #294 needed to fix something else to see that.
fix #57