Skip to content

[Console] Deprecate Helper::strlen() for width() and length() #40695

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

Merged
merged 1 commit into from
Apr 9, 2021

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Apr 3, 2021

Q A
Branch? 5.2
Bug fix? yes
New feature? yes
Deprecations? yes
Tickets Follow up form #40698
License MIT
Doc PR

This PR will deprecated Helper::strlen() since it is actually calculating the width. I remove the @internal on Helper::width() and a Helper::length(). I will also deprecate Helper::strlenWithoutDecoration() because you should use Helper::removeDecoration() instead.

Copy link
Member Author

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

There are a lot of changes in this PR, but only the two lines I've marked below is significant, the others are just renaming.

What I fail to understand is that in the Table, it feels like it should be "width" instead of "length". But "length" is the one working".

@@ -513,7 +513,7 @@ private static function initPlaceholderFormatters(): array
$completeBars = $bar->getBarOffset();
$display = str_repeat($bar->getBarCharacter(), $completeBars);
if ($completeBars < $bar->getBarWidth()) {
$emptyBars = $bar->getBarWidth() - $completeBars - Helper::strlenWithoutDecoration($output->getFormatter(), $bar->getProgressCharacter());
$emptyBars = $bar->getBarWidth() - $completeBars - Helper::strlengthWithoutDecoration($output->getFormatter(), $bar->getProgressCharacter());
Copy link
Member Author

Choose a reason for hiding this comment

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

Note this change.

@@ -511,7 +511,8 @@ private function renderCell(array $row, int $column, string $cellFormat): string
return sprintf($style->getBorderFormat(), str_repeat($style->getBorderChars()[2], $width));
}

$width += Helper::strlen($cell) - Helper::strlenWithoutDecoration($this->output->getFormatter(), $cell);
$x = 2;
$width += Helper::strlength($cell) - Helper::strlengthWithoutDecoration($this->output->getFormatter(), $cell);
Copy link
Member Author

Choose a reason for hiding this comment

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

Note these two changes.

@chalasr
Copy link
Member

chalasr commented Apr 3, 2021

We cannot deprecate something in 5.2 as it's a maintenance branch, it should be done on 5.x.
For 5.2, the breaking change made to Helper::strlen() should probably be reverted

@Nyholm
Copy link
Member Author

Nyholm commented Apr 3, 2021

Thank you. I was hesitant about this.

Are we okey with reverting #40524, adding that PR as a feature to 5.x and then merge this PR in 5.x?

@Nyholm Nyholm changed the base branch from 5.2 to 5.x April 3, 2021 11:32
@Nyholm Nyholm force-pushed the console-strlength branch from 7484158 to c9c3c01 Compare April 3, 2021 11:32
fabpot added a commit that referenced this pull request Apr 8, 2021
…, grasmash)

This PR was merged into the 5.2 branch.

Discussion
----------

[Console] Add Helper::width() and Helper::length()

| Q             | A
| ------------- | ---
| Branch?       | 5.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Close #40697 Fix #40634, fix #40635
| License       | MIT
| Doc PR        |

This PR will add add a Helper::strwidth() and a Helper::strlength(). Same with with the Helper::strlenWithoutDecoration(). It does not deprecate anything. That is done in #40695

With this PR we dont have to revert the emoji issue (ie close #40697)

FYI @grasmash, I used your tests from #40635

Commits
-------

d9ea4c5 Add test.
dc02ab3 [Console] Add Helper::strwidth() and Helper::strlength()
@Nyholm Nyholm force-pushed the console-strlength branch from c9c3c01 to 5679e99 Compare April 8, 2021 07:11
@Nyholm Nyholm changed the title [Console] Deprecate Helper::strlen() for strwidth() and strlength() [Console] Deprecate Helper::strlen() for width() and length() Apr 8, 2021
@Nyholm Nyholm added Feature and removed Bug labels Apr 8, 2021
@Nyholm Nyholm force-pushed the console-strlength branch from 5e65d3b to b883112 Compare April 8, 2021 07:34
@chalasr chalasr removed this from the 5.2 milestone Apr 8, 2021
@chalasr chalasr added this to the 5.x milestone Apr 8, 2021
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Thank you for taking care of this.

@fabpot fabpot force-pushed the console-strlength branch from 6ca1bf6 to 3c24aa9 Compare April 9, 2021 09:54
@fabpot
Copy link
Member

fabpot commented Apr 9, 2021

Thank you @Nyholm.

@Nyholm
Copy link
Member Author

Nyholm commented Apr 9, 2021

Thank you for merging

@Nyholm Nyholm deleted the console-strlength branch April 9, 2021 09:54
@fabpot fabpot mentioned this pull request Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants