Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Adds optional type argument accepted by str_pad()#120

Merged
gitlost merged 4 commits into
wp-cli:masterwp-cli/php-cli-tools:masterfrom
urlund:masterCopy head branch name to clipboard
Oct 2, 2017
Merged

Adds optional type argument accepted by str_pad()#120
gitlost merged 4 commits into
wp-cli:masterwp-cli/php-cli-tools:masterfrom
urlund:masterCopy head branch name to clipboard

Conversation

@urlund

@urlund urlund commented Sep 29, 2017

Copy link
Copy Markdown
Contributor

Issue #121

@schlessera

Copy link
Copy Markdown
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

urlund commented Oct 2, 2017

Copy link
Copy Markdown
Contributor Author

@schlessera Done :)

@gitlost

gitlost commented Oct 2, 2017

Copy link
Copy Markdown
Contributor

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

urlund commented Oct 2, 2017

Copy link
Copy Markdown
Contributor Author

Done?

@gitlost gitlost left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread tests/test-cli.php
$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
Copy Markdown
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

urlund commented Oct 2, 2017

Copy link
Copy Markdown
Contributor Author

@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

gitlost commented Oct 2, 2017

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.