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

Fixed many small phpcs issues #1228

Closed
wants to merge 1 commit into from
Closed

Conversation

acrobat
Copy link
Contributor

@acrobat acrobat commented Dec 19, 2014

This pr fixes many small phpcs issues

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3458

We use Jira to track the state of pull requests and the versions they got
included in.

@Ocramius Ocramius self-assigned this Dec 19, 2014
@Ocramius
Copy link
Member

This one has to stay on hold until the other relevant PRs (for 2.5) are merged.

@acrobat
Copy link
Contributor Author

acrobat commented Dec 19, 2014

Ok @Ocramius, just ping when we it's ready to be merged and pr needs rebase!

@@ -570,8 +570,8 @@ public function close()
*/
public function persist($entity)
{
if ( ! is_object($entity)) {
throw ORMInvalidArgumentException::invalidObject('EntityManager#persist()' , $entity);
if (!is_object($entity)) {
Copy link
Member

Choose a reason for hiding this comment

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

Doctrine uses spaces around the negation operator currently (I would vote in favor of changing this to match the PSR-2 rule though, but it is not done currently, and I'm not the one taking this decision)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh didn't know about that code convention @stof! Reverted those changes, but that's indeed something we should try to get according the PSR-2 standards in the future!

Copy link
Member

Choose a reason for hiding this comment

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

@acrobat we're actually going to keep the ! convention also in future ;-)

See https://github.com/doctrine/coding-standard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ocramius any reason why doctrine differs from the PSR-2? Of course if there is an other reason than personal taste ;)

Copy link
Member

Choose a reason for hiding this comment

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

PSR-2 is a generic coding standard that is a fit for pretty much everyone: doctrine is much stricter

Copy link
Member

Choose a reason for hiding this comment

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

@Ocramius Having stricter rules does not forbid us to change these rules to be consistent with PSR-2. The Doctrine CS would then be PSR-2 + Doctrine-specific rules. Currently, it is PSR-2 - negation formatting + Doctrine-specific rules.
For instance, Symfony also applies more rules than PSR-2 (rules which are common to the Doctrine CS for many of them btw), but it builds on top of PSR-2, and we tweaked the Symfony rules a bit when PSR-2 came out to avoid inconsistencies.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but we explicitly want to break compatibility with PSR-2 here, that's what I meant.

I am aware that it is not a superset.

Copy link
Member

Choose a reason for hiding this comment

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

My question is "why do we want to break compatibility here ?". But anyway, this should be discussed elsewhere

Copy link
Member

Choose a reason for hiding this comment

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

Always the same discussion ;) Maybe it's time to open an issue @ https://github.com/doctrine/coding-standard and have it be settled by the team once and for all.

Copy link
Member

Choose a reason for hiding this comment

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

@stof we've been doing it for ages and it has worked out really nice so far, so: why not? :-)

We do it for visibility of the negation, fwiw.

@TomasVotruba
Copy link
Contributor

This shall be ready now, since 2.5 is out

@acrobat
Copy link
Contributor Author

acrobat commented Sep 4, 2015

I rebased the branch, should be good to merge!

@Ocramius
Copy link
Member

@acrobat I rebased and applied this to master, thanks a lot!

@acrobat acrobat deleted the phpcs-fixes branch January 4, 2016 10:44
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.

6 participants