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

Update the handling of PURE_PYTHON to match zope.interface and persistest #35

Merged
merged 3 commits into from
Apr 1, 2020

Conversation

jamadden
Copy link
Member

@jamadden jamadden commented Apr 1, 2020

Specifically, we can always build the extensions, so build-time handling on PURE_PYTHON is gone. Runtime is updated to respect PURE_PYTHON=0. Tests are added for this.

  • Stop using the deprecated, untested, zope.interafce.declarations.ObjectSpecification function in favor of Provides.
  • Drop support for setup.py test
  • Add python_requires metadata

The branch name is issue34 because this started as some cleanups while working on #34, but it grew big enough to require its own PR.

…tent

Also stop using the deprecated, untested,
``zope.interafce.declarations.ObjectSpecification`` function in favor
of ``Provides``.
Copy link
Member

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

LGTM with minor comments

.travis.yml Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
src/zope/container/contained.py Outdated Show resolved Hide resolved
@jamadden jamadden merged commit 1cd4079 into master Apr 1, 2020
'zope.testing',
'zope.testrunner',
],
tests_require=extras['test'],
Copy link
Member

Choose a reason for hiding this comment

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

Weren't we dropping setup.py test everywhere? test_require is not useful for anything else.

@jamadden jamadden deleted the issue34 branch April 1, 2020 14:45
docs/conf.py Show resolved Hide resolved
@@ -26,7 +26,7 @@ commands =
coverage report --fail-under=100 --show-missing
# parallel mode: make sure all builds complete before we run this one
depends =
py27,py35,py36,py37,py38,pypy,py27-zodb,pypy-zodb,py27-pure-zodb
py27,py35,py36,py37,py38,pypy,pypy3
Copy link
Member

Choose a reason for hiding this comment

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

py27-pure should remain in here I think

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it add much value given the existence of pypy and pypy3?

Copy link
Member Author

Choose a reason for hiding this comment

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

(Note that it's still in the environment list, just not the parallel coverage dependency list.)

Copy link
Member

Choose a reason for hiding this comment

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

I dunno, do we want to run coverage combine while coverage run -p is still running on py27-pure?

I suppose pypy will cover the pure-Python alternative branches so the coverage number shouldn't drop because of this. As long as we don't have pypy-specific vs Python-pure-specific branches in the code...

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose pypy will cover the pure-Python alternative branches so the coverage number shouldn't drop because of this.

It didn't, so...

As long as we don't have pypy-specific vs Python-pure-specific branches in the code…

...I don't think we do. If we do in the future, the coverage number will drop and we can know we need to depend on it or rework the code.

@mgedmin
Copy link
Member

mgedmin commented Apr 1, 2020

The merge was maybe a little bit premature?

@jamadden
Copy link
Member Author

jamadden commented Apr 1, 2020

I apologize, I didn't realize you were reviewing again.

@mgedmin
Copy link
Member

mgedmin commented Apr 1, 2020

The merge was maybe a little bit premature?

Or maybe not.

@mgedmin
Copy link
Member

mgedmin commented Apr 1, 2020

I apologize, I didn't realize you were reviewing again.

I'm sorry, I'm unpredictable that way. There are days when I tire of banging head against unsolvable problems (Selenium test flakyness, to pick a random example) in $DAYJOB and I poll GitHub notifications for any distraction. And there are days (weeks) when I can't find the energy to review things I've been asked to review...

@mgedmin
Copy link
Member

mgedmin commented Apr 1, 2020

Some of my Selenium frustration may have leaked through, for which I apologize.

@jamadden
Copy link
Member Author

jamadden commented Apr 1, 2020

No worries! Thank you for all you do here.

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

2 participants