-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[RFC] Introduce attrs #2641
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
attrs is awesome, and would be great to have in pytest.
62742f1
to
b2f2d76
Compare
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. |
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 |
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 |
b2f2d76
to
1ff293e
Compare
If by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something but didn't attrs just get vendored as well? Why is it also unconditionally part of install_requires
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i forgot that while vendoring thanks for the note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
_pytest/__init__.py
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
5498fba
to
09a5dc7
Compare
setup.py
Outdated
'py>=1.4.33', | ||
'six>=1.10.0', | ||
'setuptools', | ||
'attrs', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably needs an appropriate version bound.
vendoring was undone, so the complaint was addressed
OK no worries. It is already a bonus to be able to use |
babe63a
to
2586dec
Compare
for FixtureFunctionMarker and marks
2586dec
to
07b2b18
Compare
Sorry just noticed now that this comment is ambiguous. Should pytest vendor |
@nicoddemus we decided not to vendor at all |
It would be great if you could add a travis job to test pytest against attrs master to prevent release drama. |
…dt/introduce-attrs
Great idea. |
Hmm now that I think about it, should we test against the master of each dependency we add? We already do that for And even testing against 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. |
Yes, testing against master of every dependency seems like a good idea
…On 4 November 2017 at 15:46, Bruno Oliveira ***@***.***> wrote:
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.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#2641 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAI9IOoSd4wvXZUFb-DMZe84WOU4hrpAks5szHjDgaJpZM4OpfXd>
.
|
if we go for testing against master of every dependency, then i'd like an new pr preparing that, then hook into it here |
There are also daily cron builds IIRC?! |
OK then sounds good. I will open another PR with that. |
The failure is unrelated, merging this then. |
@nicoddemus awesome , thanks |
If you get to it, please go ahead. I'll probably only have time tomorrow anyways. 😇 |
same here ^^, i'll open a issue |
No description provided.