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

Inline condition removed for easier reading #4387

Closed
wants to merge 3 commits into from
Closed

Inline condition removed for easier reading #4387

wants to merge 3 commits into from

Conversation

acidjames
Copy link
Contributor

Previous inline condition would cause confusion or trigger notifications in IDE (Netbeans,...)

Previous inline condition would cause confusion or trigger notifications in IDE (Netbeans,...)
@@ -404,7 +404,8 @@ Next, refactor the ``Document`` class to take advantage of these callbacks::
*/
public function removeUpload()
{
if ($file = $this->getAbsolutePath()) {
$file == $this->getAbsolutePath();
Copy link
Member

Choose a reason for hiding this comment

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

Here you have to use an assignment but no comparison operator.

Copy link
Member

Choose a reason for hiding this comment

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

(sorry, I made that typo too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha we'll get it right soon !

@acidjames
Copy link
Contributor Author

Hi i see that i referenced this PR from a commit, is that enough for it to be merged ? sorry i'm new ...

@wouterj
Copy link
Member

wouterj commented Oct 29, 2014

@acidjames you should put the required changes in the branch that is related to this PR: acidjames:patch-1

The PR can only be merged by people with push access (basically @weaverryan and me). I'll merge your PR once you did the required changed. Btw, if you need more help, don't hesitate to ask :)

@acidjames
Copy link
Contributor Author

@wouterj i've been trying to access acidjames:patch-1 with the official Github client without success, should i be using this client or something else ?

@wouterj
Copy link
Member

wouterj commented Oct 30, 2014

@acidjames you can go to http://github.com/acidjames/symfony-docs/tree/patch-1 Then navigate to the file and click on "edit", make the changes and commit them.

@@ -404,7 +404,8 @@ Next, refactor the ``Document`` class to take advantage of these callbacks::
*/
public function removeUpload()
{
if ($file = $this->getAbsolutePath()) {
$file = $this->getAbsolutePath();
+ if ($file) {
Copy link
Member

Choose a reason for hiding this comment

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

The + has to be removed.

@acidjames
Copy link
Contributor Author

Sorry guys, i'll do better next time i promise !

@wouterj
Copy link
Member

wouterj commented Oct 31, 2014

No worries, we are here to help you. You did a great job, thanks!

wouterj added a commit that referenced this pull request Oct 31, 2014
This PR was submitted for the 2.5 branch but it was merged into the 2.3 branch instead (closes #4387).

Discussion
----------

Inline condition removed for easier reading

Previous inline condition would cause confusion or trigger notifications in IDE (Netbeans,...)

Commits
-------

b98f5e8 Inline condition removed for easier reading
@wouterj wouterj closed this Oct 31, 2014
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.

3 participants