Skip to content

Conversation

@oz123
Copy link
Contributor

@oz123 oz123 commented Oct 22, 2016

@loechel still WIP ... Check the changes done. I hope this helps.

@oz123 oz123 merged commit dac38ad into zopefoundation:Python3_update Oct 23, 2016
@icemac
Copy link
Member

icemac commented Oct 24, 2016

@oz123 @loechel I am not happy with some of the changes made in this PR. Has there been a review outside this PR?

@stephan-hof
Copy link
Member

I appreciate the work of migrating all the old unittests, but merging them right now is a bit too early from my point of view.

After this merge the test framework fails. So how should I know if new changes break existing code ?
I was planning to tackle 'sequence unpacking in assignments', but now I'm distracted by all this new failing tests.

I thought the strategy is adding checks/protections + their tests step by step and always having a green bar.

@icemac
Copy link
Member

icemac commented Oct 25, 2016

@oz123 As @stephan-hof said: Thank you for your hard work in porting the tests. But now tests for each Python version are broken. So I have to revert the merge. Please split it up in smaller parts and make sure the test run successfully using tox.

@icemac icemac mentioned this pull request Oct 25, 2016
@oz123
Copy link
Contributor Author

oz123 commented Oct 25, 2016

@icemac sorry for the late reply. No problem with the revert. I will see that I can port the tests in smaller chunks.

@icemac
Copy link
Member

icemac commented Oct 25, 2016

@oz123 Fine :) The idea of porting the tests was to convert each example into a test which is both run on the old and on the new implementation. Here is an example of what I mean.

@loechel
Copy link
Member

loechel commented Oct 25, 2016

Hi together,

sorry for the late answer, I just arrived yesterday from PloneConf2016 in Boston and had directly to attend meeting sin the office.

@oz123 was working with me at the PloneConf2016 on porting additional Tests and features of RestrictedPython he did a Pull Request before I leave and asked me to review what I didn't had the chance to.

@icemac and @stephan-hof are right, that shouldn't have been merged already. It would be good if we could revert and and than reapply test and feature one by one, so that after all everybody could work on an green build. @oz123 might you at the moment please revert and keep your branch, and at the PyCon.DE we might could work on and spiltt and merge all of it step by step.

@icemac
Copy link
Member

icemac commented Oct 28, 2016

@loechel and @oz123 I already reverted the merge using #2.

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.

4 participants