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

Enhance pluralization #54

Merged
merged 2 commits into from Oct 4, 2017

Conversation

5 participants
@fumikito
Contributor

fumikito commented Sep 19, 2017

Relates to #48, added Doctrine Inflector class and replaced exiting method pluralize .

I considered adding unit test, but didn't add. Because pluralize is just an alias for Inflector's method which is fully tested.

@miya0001

This comment has been minimized.

Show comment
Hide comment
@miya0001

miya0001 Sep 19, 2017

Member

Thanks for pull-request @fumikito. 😄

@wp-cli/committers Any thoughts?

Member

miya0001 commented Sep 19, 2017

Thanks for pull-request @fumikito. 😄

@wp-cli/committers Any thoughts?

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Sep 20, 2017

Contributor

I guess the name space of the Inflector should be like WP_CLI\Inflector

That sounds good (and necessary).

We need to add props to original project

I'm not sure why they did that in core, looks OTT. The Doctrine\Inflector project is a stand-alone repo so I'd have thought what's in the class now is enough.

Do we need unit test?

I see no real need, as @fumikito says.

Contributor

gitlost commented Sep 20, 2017

I guess the name space of the Inflector should be like WP_CLI\Inflector

That sounds good (and necessary).

We need to add props to original project

I'm not sure why they did that in core, looks OTT. The Doctrine\Inflector project is a stand-alone repo so I'd have thought what's in the class now is enough.

Do we need unit test?

I see no real need, as @fumikito says.

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Sep 24, 2017

Member

I'm not sure why they did that in core, looks OTT.

The PHPMailer license bit was probably part of the original PHPMailer source code and just copied over.

In our case, the existing license text should be sufficient.

I would like to see 1 additional feature test that uses one of the inflections that were not part of the previous code. This will allow us to detect regressions where we're relying on an old function implmentation.

Member

schlessera commented Sep 24, 2017

I'm not sure why they did that in core, looks OTT.

The PHPMailer license bit was probably part of the original PHPMailer source code and just copied over.

In our case, the existing license text should be sufficient.

I would like to see 1 additional feature test that uses one of the inflections that were not part of the previous code. This will allow us to detect regressions where we're relying on an old function implmentation.

@danielbachhuber danielbachhuber self-requested a review Sep 26, 2017

@danielbachhuber

From @schlessera:

I would like to see 1 additional feature test that uses one of the inflections that were not part of the previous code. This will allow us to detect regressions where we're relying on an old function implmentation.

@danielbachhuber danielbachhuber merged commit b0374bc into wp-cli:master Oct 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Oct 4, 2017

Contributor

Shouldn't the namespace be changed as @miya0001 suggested, in case of clashing with user inclusion?

Contributor

gitlost commented Oct 4, 2017

Shouldn't the namespace be changed as @miya0001 suggested, in case of clashing with user inclusion?

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Oct 4, 2017

Member

Shouldn't the namespace be changed as @miya0001 suggested, in case of clashing with user inclusion?

Yes, good catch. Want to submit a PR?

Member

danielbachhuber commented Oct 4, 2017

Shouldn't the namespace be changed as @miya0001 suggested, in case of clashing with user inclusion?

Yes, good catch. Want to submit a PR?

@miya0001

This comment has been minimized.

Show comment
Hide comment
@miya0001

miya0001 Oct 4, 2017

Member

Ah, I will. :)

Member

miya0001 commented Oct 4, 2017

Ah, I will. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment