Skip to content
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

Rename to DataObjectTest #8839

Merged
merged 1 commit into from
Mar 11, 2017
Merged

Rename to DataObjectTest #8839

merged 1 commit into from
Mar 11, 2017

Conversation

mfdj
Copy link
Contributor

@mfdj mfdj commented Mar 10, 2017

This PR suggests two things

  1. Rename test to DataObjectTest because it tests DataObject class
  2. use PSR-2 property-name for test subject

Rationale

  1. I wanted a reference for how DataObject behaves and couldn't find a test. I was half finished writing my own test case before discovering that what I needed was ObjectTest? I think renaming it could help others.
  2. PSR-2 coding style dictates that protected/private properties should not be prefixed with an underscore

@orlangur
Copy link
Contributor

LGTM 👍 I'm surprised Go To > Test in PhpStorm cannot handle this.

Regarding leading underscore removal - please don't do it manually here and there, it would be more effective as automated change.

@ishakhsuvarov ishakhsuvarov self-assigned this Mar 10, 2017
@ishakhsuvarov ishakhsuvarov added this to the March 2017 milestone Mar 10, 2017
@mfdj
Copy link
Contributor Author

mfdj commented Mar 10, 2017

@orlangur maybe I need to setup my phpstorm?

This is what I see on "go to > test" it prompts me to create a test?

screen shot 2017-03-10 at 9 37 02 am

Update

Ok weird, it worked after reloading PhpStorm — I think it just need to reindex or something. Thanks for the tip 👍

@orlangur
Copy link
Contributor

@mfdj I mean I tried on mainline without your change and it didn't pick up wrongly named test properly.

@magento-team magento-team merged commit b3ecde6 into magento:develop Mar 11, 2017
@mfdj mfdj deleted the suggest-rename-to-dataobjectest branch March 11, 2017 00:27
@okorshenko
Copy link
Contributor

@mfdj thank you for your contribution!
@orlangur thank you for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants