Skip to content

Adds optional type argument accepted by str_pad() #120

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 4 commits into from
Oct 2, 2017
Merged

Adds optional type argument accepted by str_pad() #120

merged 4 commits into from
Oct 2, 2017

Conversation

urlund
Copy link
Contributor

@urlund urlund commented Sep 29, 2017

Issue #121

@schlessera
Copy link
Member

@urlund Before submitting a pull request, could you create an issue first describing the problem you're trying to solve?

See the Handbook section about pull requests for more details about our processes.

@urlund
Copy link
Contributor Author

urlund commented Oct 2, 2017

@schlessera Done :)

@gitlost
Copy link
Contributor

gitlost commented Oct 2, 2017

Thanks for the PR @urlund.

I think it would be better (simpler at least) to only support $pad_type, as $pad_string requires allowing for varying length strings, and I don't see much application for it anyway.

Also could you add phpunit tests please? (See https://github.com/wp-cli/php-cli-tools/blob/master/tests/test-cli.php#L41).

(Just to note: #122 will have to be merged once it's approved for the tests to pass on Travis.)

@urlund
Copy link
Contributor Author

urlund commented Oct 2, 2017

Done?

Copy link
Contributor

@gitlost gitlost left a comment

Choose a reason for hiding this comment

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

Thanks @urlund, just some extra tests and it's good to go.

@@ -44,6 +44,9 @@ function test_encoded_string_pad() {
$this->assertEquals( 7, strlen( \cli\Colors::pad( 'óra', 6 ) ) ); // special characters take one byte
$this->assertEquals( 9, strlen( \cli\Colors::pad( '日本語', 6 ) ) ); // each character takes two bytes
$this->assertEquals( 17, strlen( \cli\Colors::pad( 'עִבְרִית', 6 ) ) ); // process Hebrew vowels
$this->assertEquals( 6, strlen( \cli\Colors::pad( 'hello', 6, false, false, STR_PAD_RIGHT ) ) );
$this->assertEquals( 7, strlen( \cli\Colors::pad( 'óra', 6, false, false, STR_PAD_LEFT ) ) ); // special characters take one byte
$this->assertEquals( 9, strlen( \cli\Colors::pad( '日本語', 6, false, false, STR_PAD_BOTH ) ) ); // each character takes two bytes
}
Copy link
Contributor

Choose a reason for hiding this comment

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

That's cool. Could you also add some tests to exercise the new functionality, eg

$this->assertSame( 5, strpos( \cli\Colors::pad( 'hello', 7, false, false, STR_PAD_RIGHT ), ' ' ) );
$this->assertSame( 6, strrpos( \cli\Colors::pad( 'hello', 7, false, false, STR_PAD_RIGHT ), ' ' ) );
$this->assertSame( 0, strpos( \cli\Colors::pad( 'hello', 7, false, false, STR_PAD_LEFT ), ' ' ) );
$this->assertSame( 1, strrpos( \cli\Colors::pad( 'hello', 7, false, false, STR_PAD_LEFT ), ' ' ) );
$this->assertSame( 0, strpos( \cli\Colors::pad( 'hello', 7, false, false, STR_PAD_BOTH ), ' ' ) );
$this->assertSame( 6, strrpos( \cli\Colors::pad( 'hello', 7, false, false, STR_PAD_BOTH ), ' ' ) );
$this->assertSame( \cli\Colors::pad( 'hello', 7 ), \cli\Colors::pad( 'hello', 7, false, false, STR_PAD_RIGHT ) );

@urlund
Copy link
Contributor Author

urlund commented Oct 2, 2017

@gitlost better? ;)

@gitlost gitlost merged commit 14b8a08 into wp-cli:master Oct 2, 2017
@gitlost gitlost added this to the 0.11.7 milestone Oct 2, 2017
@gitlost
Copy link
Contributor

gitlost commented Oct 2, 2017

@urlund Oh yes!

@danielbachhuber danielbachhuber changed the title Added str_pad optional arguments Adds optional arguments accepted by str_pad() Oct 2, 2017
@danielbachhuber
Copy link
Member

@urlund urlund changed the title Adds optional arguments accepted by str_pad() Adds optional type argument accepted by str_pad() Oct 2, 2017
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