[RFC] Introduce attrs #2641

RonnyPfannschmidt · 2017-08-01T09:53:16Z

No description provided.

blueyed

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

The-Compiler · 2017-08-01T10:38:11Z

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 · 2017-08-01T10:43:28Z

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 · 2017-08-01T10:45:12Z

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

nicoddemus · 2017-08-01T10:57:06Z

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

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.

flub · 2017-08-01T11:25:05Z

setup.py

+ install_requires = [
+ 'py>=1.4.33',
+ 'setuptools',
+ 'attrs',


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

i forgot that while vendoring thanks for the note

jaraco

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.

jaraco · 2017-08-01T11:48:28Z

_pytest/__init__.py

@@ -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


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.

jaraco · 2017-08-01T11:56:24Z

_pytest/vendored_packages/attr/__init__.py

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


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?

@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

The-Compiler · 2017-10-10T16:31:30Z

setup.py

+ 'py>=1.4.33',
+ 'six>=1.10.0',
+ 'setuptools',
+ 'attrs',


Probably needs an appropriate version bound.

vendoring was undone, so the complaint was addressed

nicoddemus · 2017-10-21T21:34:45Z

there are ltos of them, but if i do a bug bang convert, all things will break interestingly

OK no worries. It is already a bonus to be able to use attrs for new code. 👍

coveralls · 2017-10-24T14:36:52Z

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

coveralls · 2017-10-24T15:16:58Z

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

for FixtureFunctionMarker and marks

coveralls · 2017-10-30T16:43:13Z

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

nicoddemus · 2017-11-03T19:10:46Z

@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 · 2017-11-03T20:31:38Z

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

RonnyPfannschmidt · 2017-11-04T07:10:46Z

@nicoddemus we decided not to vendor at all

hynek · 2017-11-04T09:46:38Z

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

…dt/introduce-attrs

nicoddemus · 2017-11-04T14:33:37Z

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

Great idea.

nicoddemus · 2017-11-04T14:46:26Z

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 · 2017-11-04T14:56:41Z

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

flub · 2017-11-06T07:44:55Z

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> .

RonnyPfannschmidt · 2017-11-06T08:36:52Z

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 · 2017-11-06T09:21:36Z

@nicoddemus

we only build on PRs and merges after all

There are also daily cron builds IIRC?!

nicoddemus · 2017-11-06T14:56:37Z

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 · 2017-11-06T14:56:53Z

The failure is unrelated, merging this then.

RonnyPfannschmidt · 2017-11-06T15:02:24Z

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

nicoddemus · 2017-11-06T15:08:45Z

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

RonnyPfannschmidt · 2017-11-06T15:52:11Z

same here ^^, i'll open a issue

RonnyPfannschmidt changed the base branch from master to features August 1, 2017 09:53

RonnyPfannschmidt requested review from pelme, hpk42, flub, The-Compiler, nicoddemus, blueyed, davehunt, jaraco, pfctdayelise, hackebrot and a team August 1, 2017 09:53

blueyed reviewed Aug 1, 2017

View reviewed changes

RonnyPfannschmidt force-pushed the introduce-attrs branch from 62742f1 to b2f2d76 Compare August 1, 2017 10:34

RonnyPfannschmidt force-pushed the introduce-attrs branch from b2f2d76 to 1ff293e Compare August 1, 2017 10:56

flub approved these changes Aug 1, 2017

View reviewed changes

pytest-dev deleted a comment from coveralls Aug 1, 2017

jaraco mentioned this pull request Aug 1, 2017

Vendoring pluggy? #944

Closed

jaraco previously requested changes Aug 1, 2017

View reviewed changes

nicoddemus mentioned this pull request Aug 2, 2017

Introduce six to the codebase to deal with Python 2/Python 3 differences #2642

Closed

RonnyPfannschmidt force-pushed the introduce-attrs branch 2 times, most recently from 5498fba to 09a5dc7 Compare October 10, 2017 16:28

The-Compiler reviewed Oct 10, 2017

View reviewed changes

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

RonnyPfannschmidt closed this Oct 24, 2017

RonnyPfannschmidt reopened this Oct 24, 2017

RonnyPfannschmidt force-pushed the introduce-attrs branch from babe63a to 2586dec Compare October 24, 2017 14:48

introduce attrs as dependency and use it

07b2b18

for FixtureFunctionMarker and marks

RonnyPfannschmidt force-pushed the introduce-attrs branch from 2586dec to 07b2b18 Compare October 30, 2017 16:04

Add CHANGELOG entry for attrs module dependency

e58e8fa

Merge remote-tracking branch 'upstream/features' into RonnyPfannschmi…

e351976

…dt/introduce-attrs

nicoddemus merged commit c33074c into pytest-dev:features Nov 6, 2017

RonnyPfannschmidt mentioned this pull request Nov 6, 2017

introduce ci test envs and cronjobs against attr master #2898

Closed

RonnyPfannschmidt deleted the introduce-attrs branch November 13, 2017 10:05

sbesson mentioned this pull request Dec 4, 2017

Limit pytest<3.3 for Python 2.6 support ome/scc#222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Introduce attrs #2641

[RFC] Introduce attrs #2641

RonnyPfannschmidt commented Aug 1, 2017

blueyed left a comment

The-Compiler commented Aug 1, 2017

RonnyPfannschmidt commented Aug 1, 2017

RonnyPfannschmidt commented Aug 1, 2017

nicoddemus commented Aug 1, 2017

flub left a comment

flub Aug 1, 2017

RonnyPfannschmidt Aug 1, 2017

jaraco left a comment

jaraco Aug 1, 2017

jaraco Aug 1, 2017

RonnyPfannschmidt Aug 1, 2017

The-Compiler Oct 10, 2017

nicoddemus commented Oct 21, 2017

coveralls commented Oct 24, 2017

coveralls commented Oct 24, 2017

coveralls commented Oct 30, 2017

nicoddemus commented Nov 3, 2017

coveralls commented Nov 3, 2017

RonnyPfannschmidt commented Nov 4, 2017

hynek commented Nov 4, 2017

nicoddemus commented Nov 4, 2017

nicoddemus commented Nov 4, 2017 •

edited

Loading

coveralls commented Nov 4, 2017

flub commented Nov 6, 2017 via email

RonnyPfannschmidt commented Nov 6, 2017

blueyed commented Nov 6, 2017

nicoddemus commented Nov 6, 2017

nicoddemus commented Nov 6, 2017

RonnyPfannschmidt commented Nov 6, 2017

nicoddemus commented Nov 6, 2017

RonnyPfannschmidt commented Nov 6, 2017

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

[RFC] Introduce attrs #2641

[RFC] Introduce attrs #2641

Conversation

RonnyPfannschmidt commented Aug 1, 2017

blueyed left a comment

Choose a reason for hiding this comment

The-Compiler commented Aug 1, 2017

RonnyPfannschmidt commented Aug 1, 2017

RonnyPfannschmidt commented Aug 1, 2017

nicoddemus commented Aug 1, 2017

flub left a comment

Choose a reason for hiding this comment

flub Aug 1, 2017

Choose a reason for hiding this comment

RonnyPfannschmidt Aug 1, 2017

Choose a reason for hiding this comment

jaraco left a comment

Choose a reason for hiding this comment

jaraco Aug 1, 2017

Choose a reason for hiding this comment

jaraco Aug 1, 2017

Choose a reason for hiding this comment

RonnyPfannschmidt Aug 1, 2017

Choose a reason for hiding this comment

The-Compiler Oct 10, 2017

Choose a reason for hiding this comment

nicoddemus commented Oct 21, 2017

coveralls commented Oct 24, 2017

coveralls commented Oct 24, 2017

coveralls commented Oct 30, 2017

nicoddemus commented Nov 3, 2017

coveralls commented Nov 3, 2017

RonnyPfannschmidt commented Nov 4, 2017

hynek commented Nov 4, 2017

nicoddemus commented Nov 4, 2017

nicoddemus commented Nov 4, 2017 • edited Loading

coveralls commented Nov 4, 2017

flub commented Nov 6, 2017 via email

RonnyPfannschmidt commented Nov 6, 2017

blueyed commented Nov 6, 2017

nicoddemus commented Nov 6, 2017

nicoddemus commented Nov 6, 2017

RonnyPfannschmidt commented Nov 6, 2017

nicoddemus commented Nov 6, 2017

RonnyPfannschmidt commented Nov 6, 2017

nicoddemus commented Nov 4, 2017 •

edited

Loading