Skip to content

Improve to display long description which has reference links#4444

Merged
gitlost merged 14 commits intomasterfrom
patch-4336
Oct 19, 2017
Merged

Improve to display long description which has reference links#4444
gitlost merged 14 commits intomasterfrom
patch-4336

Conversation

@miya0001
Copy link
Member

@miya0001 miya0001 commented Oct 16, 2017 •
edited by danielbachhuber
Loading

Fixes #4336

It will be like following for now.

$ bin/wp help role

NAME

  wp role

DESCRIPTION

  Manages user roles, including creating new roles and resetting to defaults.

SYNOPSIS

  wp role <command>

SUBCOMMANDS

  create      Create a new role.
  delete      Delete an existing role.
  exists      Check if a role exists.
  list        List all roles.
  reset       Reset any default role to default capabilities.

  See references for [Roles and Capabilities][1] and [WP User class][2].
  ---
  [1] https://codex.wordpress.org/Roles_and_Capabilities
  [2] https://codex.wordpress.org/Class_Reference/WP_User


EXAMPLES

...

@danielbachhuber
Copy link
Member

danielbachhuber commented Oct 16, 2017

Just so it's stated, let's refrain from merging this until v1.4.0 has landed.

@miya0001 miya0001 requested a review from a team October 16, 2017 13:51
@miya0001
Copy link
Member Author

miya0001 commented Oct 16, 2017

Just so it's stated, let's refrain from merging this until v1.4.0 has landed.

Agreed 😊

@miya0001 miya0001 removed the request for review from a team October 16, 2017 13:59
@miya0001 miya0001 requested a review from a team October 17, 2017 16:06
danielbachhuber
danielbachhuber requested changes Oct 17, 2017
View reviewed changes
php/WP_CLI/DocParser.php Outdated
break;
}

if ( preg_match_all( '/(\[.+?\]\((https?:\/\/.+?)\))/', $line, $m ) ) {
Copy link
Member

@danielbachhuber danielbachhuber Oct 17, 2017

Choose a reason for hiding this comment

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

Could we get a plain phpunit test for this too? There's an existing TestDocParser class you can add to.

@miya0001
Copy link
Member Author

miya0001 commented Oct 19, 2017

@danielbachhuber I did. 😊

@miya0001
Copy link
Member Author

miya0001 commented Oct 19, 2017

I changed the order of arguments of implode().

it may be less confusing to use the documented order of arguments.
http://php.net/manual/en/function.implode.php

@danielbachhuber danielbachhuber added this to the 1.5.0 milestone Oct 19, 2017
@danielbachhuber
Copy link
Member

danielbachhuber commented Oct 19, 2017

👍 Looks good to me. Requesting a second review from @gitlost

@gitlost
Copy link
Contributor

gitlost commented Oct 19, 2017

Looks really nice and works well. A few nitpicks, all ignorable, would be that the regex doesn't need the all-including brackets so could be simplified to '/\[.+?\]\((https?:\/\/.+?)\)/', as $matches isn't needed and then $references = $m[1]. Also in general a count() should be assigned and not used in a loop test expression.

(Also overall you could argue that preg_replace_callback() is a more natural fit here, as str_replace() could theoretically replace more than it should. But it's highly unlikely.)

@gitlost gitlost merged commit 537eac4 into master Oct 19, 2017
@gitlost gitlost deleted the patch-4336 branch October 19, 2017 18:08
@danielbachhuber danielbachhuber mentioned this pull request Oct 24, 2017
Merged
@danielbachhuber danielbachhuber removed this from the 1.5.0 milestone Oct 24, 2017
@miya0001 miya0001 mentioned this pull request Oct 25, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danielbachhuber danielbachhuber danielbachhuber approved these changes

+1 more reviewer

@gitlost gitlost gitlost approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

scope:documentation Related to documentation scope:framework

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@miya0001 @danielbachhuber @gitlost