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

[RFC] Introduce attrs #2641

Merged
merged 3 commits into from Nov 6, 2017
Merged

Conversation

@RonnyPfannschmidt
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt commented Aug 1, 2017

No description provided.

@RonnyPfannschmidt RonnyPfannschmidt changed the base branch from master to features Aug 1, 2017
Copy link
Contributor

@blueyed blueyed left a comment

👍
attrs is awesome, and would be great to have in pytest.

@RonnyPfannschmidt RonnyPfannschmidt force-pushed the RonnyPfannschmidt:introduce-attrs branch from 62742f1 to b2f2d76 Aug 1, 2017
@The-Compiler
Copy link
Member

@The-Compiler The-Compiler commented Aug 1, 2017

Note that attrs doesn't support Python 2.6 (which we said we would support until pip doesn't). I'm also not sure what this means for @hynek's usage of pytest to test attrs.

@RonnyPfannschmidt
Copy link
Member Author

@RonnyPfannschmidt RonnyPfannschmidt commented Aug 1, 2017

i care about python2.6 a lot less than sorting out cruft, and as far as i can tell the next feature release after 3.2 can drop python2.6 entirely

@RonnyPfannschmidt
Copy link
Member Author

@RonnyPfannschmidt RonnyPfannschmidt commented Aug 1, 2017

wrt vendoring i just verified with @hynek on irc that vendoring is the better approach in the pytest case in order to ensure sound testing

@RonnyPfannschmidt RonnyPfannschmidt force-pushed the RonnyPfannschmidt:introduce-attrs branch from b2f2d76 to 1ff293e Aug 1, 2017
@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Aug 1, 2017

i care about python2.6 a lot less than sorting out cruft, and as far as i can tell the next feature release after 3.2 can drop python2.6 entirely

If by pytest-3.3 pip has released 10.0, then I agree completely (including dropping support for python 3.3 as well). Otherwise I would not like to break our promise of only dropping support when pip does.

@flub
flub approved these changes Aug 1, 2017
Copy link
Member

@flub flub left a comment

My gut instinct is to be -0 on this as a whole. But I'm sure you've all done your homework and it's worth it so I'll change that to +0.

setup.py Outdated
install_requires = [
'py>=1.4.33',
'setuptools',
'attrs',

This comment has been minimized.

@flub

flub Aug 1, 2017
Member

I might be missing something but didn't attrs just get vendored as well? Why is it also unconditionally part of install_requires?

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Aug 1, 2017
Author Member

i forgot that while vendoring thanks for the note

@pytest-dev pytest-dev deleted a comment from coveralls Aug 1, 2017
@pytest-dev pytest-dev deleted a comment from coveralls Aug 1, 2017
@jaraco jaraco mentioned this pull request Aug 1, 2017
Copy link
Contributor

@jaraco jaraco left a comment

I believe there's a stale comment in the vendored packages README. I suggest it should be updated to include the motivations for vendoring attrs.

@@ -6,3 +6,5 @@
# broken installation, we don't even try
# unknown only works because we do poor mans version compare
__version__ = 'unknown'

from .vendored_packages import attr

This comment has been minimized.

@jaraco

jaraco Aug 1, 2017
Contributor

I'm a little wary of this approach of importing a module simply so it's exposed for other modules to import. It may work for attrs because attrs happens to import all of its submodules, but in the general case, this pattern doesn't work. I considered something like this when vendoring packaging into setuptools, but packaging doesn't import all of its submodules, so an from ._vendor import packaging doesn't expose packaging.specifiers (or simililar), but moreover, it's difficult for another module that needs those submodules to make the submodules available.

This pattern is discouraged for exposing non-vendored functionality, so I think it should be discouraged for vendored as well.

@@ -0,0 +1,71 @@
from __future__ import absolute_import, division, print_function

This comment has been minimized.

@jaraco

jaraco Aug 1, 2017
Contributor

I'm strongly opposed to any vendoring if it can be avoided. I read up on #944, which explains the motivations for vendoring pluggy. Do those same motivations apply for this package? Do those motivations not apply to any package on which pytest depends?

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Aug 1, 2017
Author Member

@jaraco vendoring atts was suggested as attrs is tested using pytest - and well the only supported way in python to import multiple versions of a package at the same time is to put them under a new name to begin with - even setuptools didnt solve that as multi version install never supported multi version import

@RonnyPfannschmidt RonnyPfannschmidt force-pushed the RonnyPfannschmidt:introduce-attrs branch 2 times, most recently from 5498fba to 09a5dc7 Oct 10, 2017
setup.py Outdated
'py>=1.4.33',
'six>=1.10.0',
'setuptools',
'attrs',

This comment has been minimized.

@The-Compiler

The-Compiler Oct 10, 2017
Member

Probably needs an appropriate version bound.

@RonnyPfannschmidt RonnyPfannschmidt dismissed jaraco’s stale review Oct 10, 2017

vendoring was undone, so the complaint was addressed

@pytest-dev pytest-dev deleted a comment from coveralls Oct 10, 2017
@coveralls
Copy link

@coveralls coveralls commented Oct 24, 2017

Coverage Status

Coverage decreased (-0.0005%) to 92.653% when pulling babe63a on RonnyPfannschmidt:introduce-attrs into 083084f on pytest-dev:features.

@RonnyPfannschmidt RonnyPfannschmidt force-pushed the RonnyPfannschmidt:introduce-attrs branch from babe63a to 2586dec Oct 24, 2017
@coveralls
Copy link

@coveralls coveralls commented Oct 24, 2017

Coverage Status

Coverage decreased (-0.0005%) to 92.653% when pulling 2586dec on RonnyPfannschmidt:introduce-attrs into 083084f on pytest-dev:features.

for FixtureFunctionMarker and marks
@RonnyPfannschmidt RonnyPfannschmidt force-pushed the RonnyPfannschmidt:introduce-attrs branch from 2586dec to 07b2b18 Oct 30, 2017
@coveralls
Copy link

@coveralls coveralls commented Oct 30, 2017

Coverage Status

Coverage decreased (-0.0005%) to 92.68% when pulling 07b2b18 on RonnyPfannschmidt:introduce-attrs into cb30848 on pytest-dev:features.

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Nov 3, 2017

@RonnyPfannschmidt

wrt vendoring i just verified with @hynek on irc that vendoring is the better approach in the pytest case in order to ensure sound testing

Sorry just noticed now that this comment is ambiguous. Should pytest vendor attrs or the other way around? Just to make sure before we merge this.

@coveralls
Copy link

@coveralls coveralls commented Nov 3, 2017

Coverage Status

Coverage increased (+0.008%) to 92.689% when pulling e58e8fa on RonnyPfannschmidt:introduce-attrs into cb30848 on pytest-dev:features.

@RonnyPfannschmidt
Copy link
Member Author

@RonnyPfannschmidt RonnyPfannschmidt commented Nov 4, 2017

@nicoddemus we decided not to vendor at all

@hynek
Copy link
Contributor

@hynek hynek commented Nov 4, 2017

It would be great if you could add a travis job to test pytest against attrs master to prevent release drama.

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Nov 4, 2017

It would be great if you could add a travis job to test pytest against attrs master to prevent release drama.

Great idea.

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Nov 4, 2017

Hmm now that I think about it, should we test against the master of each dependency we add? We already do that for pluggy, exactly because we almost made a new pluggy release which by accident would break pytest. The same can be true for any of the new dependencies: attrs, funcsigs or even colorama.

And even testing against master is not 100% safe, consider this situation: someone merges a funcsigs PR (just an example, could be any of the dependencies) and makes a release. Our funcsigs jobs would break with the new master, but in the meantime between the merge and the release no pytest builds have happened (we only build on PRs and merges after all). In this situation it would immediately break pytest.

Of course adding dependencies to a project you add the risk of breaking because of an update on a dependency, it is the trade-off of not having to implement everything yourself.

To be clear: I'm definitely not against adding new libraries, but we should have a clear policy if we will test all dependencies against their development branch or not.

@coveralls
Copy link

@coveralls coveralls commented Nov 4, 2017

Coverage Status

Coverage decreased (-0.0005%) to 92.683% when pulling e351976 on RonnyPfannschmidt:introduce-attrs into b18a9de on pytest-dev:features.

@flub
Copy link
Member

@flub flub commented Nov 6, 2017

@RonnyPfannschmidt
Copy link
Member Author

@RonnyPfannschmidt RonnyPfannschmidt commented Nov 6, 2017

if we go for testing against master of every dependency, then i'd like an new pr preparing that, then hook into it here

@blueyed
Copy link
Contributor

@blueyed blueyed commented Nov 6, 2017

@nicoddemus

we only build on PRs and merges after all

There are also daily cron builds IIRC?!

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Nov 6, 2017

if we go for testing against master of every dependency, then i'd like an new pr preparing that, then hook into it here

OK then sounds good. I will open another PR with that.

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Nov 6, 2017

The failure is unrelated, merging this then.

@nicoddemus nicoddemus merged commit c33074c into pytest-dev:features Nov 6, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@RonnyPfannschmidt
Copy link
Member Author

@RonnyPfannschmidt RonnyPfannschmidt commented Nov 6, 2017

@nicoddemus awesome , thanks
i was preparing myself for it but i keep getting interrupted today

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Nov 6, 2017

If you get to it, please go ahead. I'll probably only have time tomorrow anyways. 😇

@RonnyPfannschmidt
Copy link
Member Author

@RonnyPfannschmidt RonnyPfannschmidt commented Nov 6, 2017

same here ^^, i'll open a issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants