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

Prepare for Python 2 / 3 compatibility #23

Merged
merged 34 commits into from
Jan 4, 2019

Conversation

andreasma
Copy link
Member

No description provided.

Copy link
Contributor

@gforcada gforcada left a comment

Choose a reason for hiding this comment

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

The changes look good, but they need some further work to get them merged 😃

travis.cfg Outdated Show resolved Hide resolved
.idea/vcs.xml Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@andreasma
Copy link
Member Author

@gforcada thanks for the hints. I changed this with my last commit, but the test with travis-ci didn't work further. There seemed to be an issue with the bootstrap.py and setuptools.

@gforcada
Copy link
Contributor

I would strongly suggest to move to pip to install setuptools and zc.buildout

You might copy the Travis setup from other plone packages, like plone.restapi for example

Copy link
Contributor

@gforcada gforcada left a comment

Choose a reason for hiding this comment

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

It looks much better, but some polishing is still needed before it can get merged, only a few minor glitches though. 😃

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
plone-5.2.x.cfg Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jan 3, 2019

Coverage Status

Coverage remained the same at ?% when pulling e467559 on andreasma:python3 into 2179e20 on collective:master.

Copy link
Contributor

@gforcada gforcada left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1,5 +1,11 @@
[buildout]
extends =
https://raw.githubusercontent.com/collective/buildout.plonetest/master/travis-4.x.cfg
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems that without this line, there is 4.3.x.cfg as well as 5.1.x.cfg and 5.2.x.cfg there is no test part being installed and thus tests can not run 😅

Doing a sed replacement here as well for this line would be my suggested change.

@andreasma
Copy link
Member Author

@gforcada there is an issue with the version of cioppino.twothumbs that is used for the instance / test. It uses version 2.1.1 and not 2.1.2-dev from github.com. Is there a suggestion how I could fix this? In my local environment I pinned the repository in the versions section of my local.cfg.

@gforcada
Copy link
Contributor

gforcada commented Jan 4, 2019

@andreasma you are probably missing develop = . on the [buildout] section of one of the buildout configuration files.

Copy link
Contributor

@gforcada gforcada left a comment

Choose a reason for hiding this comment

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

Cool!

@gforcada gforcada merged commit 7a7878b into collective:master Jan 4, 2019
@gforcada
Copy link
Contributor

gforcada commented Jan 4, 2019

@andreasma congrats!! 🎉 would you like to get it released, or are you planning to add more changes? 🤔

@andreasma
Copy link
Member Author

@gforcada thanks for your great support! I'm currently not planning further changes. I removed only the travis entry for building of Plone 5.0. It'd be great if you could merge that change too.
It'd be fine, if we could a release from that state.
I'll look at further collective packages and try to migrate them to Python 3, e.g. dexteritytextindexer and clamav.

@gforcada
Copy link
Contributor

gforcada commented Jan 5, 2019

@andreasma 2.1.2 is out, give it a try and report any problem with it 😉

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.

None yet

3 participants