Skip to content
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

Align the percentage output #140

Merged

Conversation

robertology
Copy link

@robertology robertology commented Apr 21, 2023

Left pad with whitespace so that the 100% line doesn't pop out of alignment.

Before:

............................................................  60/337 (17 %)
............................................................ 120/337 (35 %)
............................................................ 180/337 (53 %)
............................................................ 240/337 (71 %)
............................................................ 300/337 (89 %)
.....................................                        337/337 (100 %)

After:

............................................................  60/337 ( 17%)
............................................................ 120/337 ( 35%)
............................................................ 180/337 ( 53%)
............................................................ 240/337 ( 71%)
............................................................ 300/337 ( 89%)
.....................................                        337/337 (100%)

Left pad with whitespace so that the 100% line doesn't pop out of alignment.
Copy link
Collaborator

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robertology Thanks for the PR.

This change makes sense to me and looks correctly implemented.
Note: I haven't tested it locally.

Review notes:

  • The 3 passed to stringWidth() for the $width parameter is redundant as it is the default value, but I kind of like it being explicitly passed as it makes it easier to understand the code.
  • This PR removes the space between the $percent and the actual % sign. I'm ambivalent about this, but it is a change which is not necessarily needed to fix the issue this PR is trying to address.

I have a feeling there are no tests currently covering this change. Would now be a good time to add some tests ?

@grogy
Copy link
Member

grogy commented Sep 26, 2023

@jrfnl I suggest merge it with one change - add space between percent value and percent mark. What do you mean?

@jrfnl
Copy link
Collaborator

jrfnl commented Sep 26, 2023

@jrfnl I suggest merge it with one change - add space between percent value and percent mark. What do you mean?

Sounds good to me.

@robertology
Copy link
Author

Sorry, I never got back to you on the comment from months ago.

For the space before the %, I would say that is extremely unusual. Just like I wouldn't expect to see $ 15.34; there usually isn't whitespace separating those symbols. The changes here have the output matching the style of the phpunit output. But if you'd like to have the additional space in there, I can put it back.

And by the way, if I recall correctly, I put the explicit 3 in there because I saw it done elsewhere in the code and that was an attempt to match the existing codebase.

@grogy
Copy link
Member

grogy commented Oct 2, 2023

Thank you, your opinion sounds good. I merge it

@grogy grogy merged commit 7ce7891 into php-parallel-lint:develop Oct 2, 2023
15 of 16 checks passed
@jrfnl jrfnl added this to the 2.0.0 milestone Mar 16, 2024
@jrfnl jrfnl modified the milestones: 2.0.0, 1.3.x Next Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants