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

Return removed children in _removeAll #9031

Closed
wants to merge 1 commit into from
Closed

Conversation

level420
Copy link
Member

@level420 level420 commented Jun 1, 2016

@level420
Copy link
Member Author

level420 commented Jun 1, 2016

Sorry! Accidently created a branch in https://github.com/qooxdoo/qooxdoo

@oetiker
Copy link
Member

oetiker commented Jun 1, 2016

looks good ... since adding a return value to a function that did NOT have a return value, should not have any negative impact IMHO ... but lets see if the tests agree

@cajus
Copy link
Contributor

cajus commented Jun 1, 2016

+1 from here, too

@level420
Copy link
Member Author

level420 commented Jun 1, 2016

I meant the fact that I created a branch in qooxdoo/qooxdoo and not in my fork. Is this something odd?

@oetiker
Copy link
Member

oetiker commented Jun 1, 2016

you can just remove the branch after merging no harm done

@cajus
Copy link
Contributor

cajus commented Jun 1, 2016

Just remove the branch after it's merged. I don't see a problem there.

@oetiker
Copy link
Member

oetiker commented Jun 1, 2016

@level420
Copy link
Member Author

level420 commented Jun 1, 2016

I was worried about the two travis runs.

@oetiker
Copy link
Member

oetiker commented Jun 1, 2016

one is for the new branch and one is for the PR

@oetiker
Copy link
Member

oetiker commented Jun 1, 2016

what IS interesting that the new branch caused a rebuild of the website ... @cajus

@woprandi
Copy link
Contributor

woprandi commented Jun 1, 2016

You should maybe add a unit test for that

@oetiker
Copy link
Member

oetiker commented Jun 1, 2016

then the coverage would not drop :)

@level420
Copy link
Member Author

level420 commented Jun 1, 2016

Yes! I will add a unit test for that asap. But now from my fork :-(

@woprandi
Copy link
Contributor

woprandi commented Jun 1, 2016

Yeah, and also because it is a good practice to always add a unit test to prevent a bug to come-back :)

@level420
Copy link
Member Author

level420 commented Jun 1, 2016

So should I create a new PR based on my fork including the test or just merge this and create another PR with the test?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.003%) to 55.639% when pulling 128178b on level420-issue-9030 into e0530f4 on master.

@level420
Copy link
Member Author

level420 commented Jun 1, 2016

Closing this one and creating a new one based on my fork.

@level420 level420 closed this Jun 1, 2016
@level420 level420 deleted the level420-issue-9030 branch June 1, 2016 08:22
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.

qx.ui.toolbar.Toolbar.removeAll() doesn't return children array
5 participants