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

Add return to removeMethodTemplate #1407

Merged
merged 2 commits into from
Nov 7, 2015
Merged

Add return to removeMethodTemplate #1407

merged 2 commits into from
Nov 7, 2015

Conversation

aivus
Copy link
Contributor

@aivus aivus commented May 13, 2015

I think we should return result of remove operation

@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-3734

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

@Ocramius
Copy link
Member

Why would that be required?

@aivus
Copy link
Contributor Author

aivus commented May 13, 2015

ArrayCollection return result of remove operation
https://github.com/doctrine/collections/blob/master/lib/Doctrine/Common/Collections/ArrayCollection.php#L119

I want to know whether the item was deleted.

@Ocramius
Copy link
Member

@aivus can you then also update the docblock as per the Collection interface?

@aivus
Copy link
Contributor Author

aivus commented May 13, 2015

return to docblock added

@aivus
Copy link
Contributor Author

aivus commented Sep 8, 2015

@Ocramius should I do anything else?

@@ -277,10 +277,12 @@ public function <methodName>(<methodTypeHint>$<variableName>)
* <description>
*
* @param <variableType> $<variableName>
*
* @return boolean TRUE if this collection contained the specified element, FALSE otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

From a PSR-5 compat perspective, this should be bool not boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I don't care tbh, was just nitpicking because there was a PR about this kinda thing recently...

@deeky666
Copy link
Member

deeky666 commented Sep 8, 2015

Might be nice to have a regression test for this here

Add tests for return types for addXxx() and removeXxx() methods
@aivus
Copy link
Contributor Author

aivus commented Oct 17, 2015

PR updated, tests added.

@aivus
Copy link
Contributor Author

aivus commented Oct 22, 2015

@Ocramius PR is ready. Please, review when you can.

@Ocramius Ocramius self-assigned this Nov 7, 2015
Ocramius added a commit that referenced this pull request Nov 7, 2015
Add return to removeMethodTemplate
@Ocramius Ocramius merged commit 28cebec into doctrine:master Nov 7, 2015
@Ocramius
Copy link
Member

Ocramius commented Nov 7, 2015

@aivus thanks!

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.

5 participants