rust-lang
Fix line number 0 unaligned display
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
@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
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
I feel it is a little difficult to put refactor commits in advance. So I I put them at the end instead.
@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.
@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 7ffe4b23d7890494468897d5d3773b352cd79bdb 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.
@epage Please allow me to respond to this PR one more time. Although I still was not able to fully understand and meet the requirements you gave, I learned a lot throughout the process. Especially how to organize better commits.
I hope I will have the chance to learn from you again in the future.
I sincerely thank you from the bottom of my heart
As I said, this appears to be fixed already. I'm assuming it's from swapping out the entire renderer recently for rustc's. Look forward to future contributions though!