no longer consider properties for fixtures of plugin objects (manages warning triggers) #6898
Conversation
|
deferring this pr as the "bugfix" is postentially a breaking change, the regendoc will have to have the warning |
| @@ -1413,6 +1413,11 @@ def parsefactories(self, node_or_obj, nodeid=NOTSET, unittest=False): | |||
| for name in dir(holderobj): | |||
| # The attribute can be an arbitrary descriptor, so the attribute | |||
| # access below can raise. safe_getatt() ignores such exceptions. | |||
| # additionally properties are ignored by default to avoid triggering warnings | |||
There was a problem hiding this comment.
For reference, the safe_getattr() and comment above was added in 3a0a0c2 to fix #2234. There I wrote
A better solution would be to completely skip access to all custom descriptors, such that the offending code doesn't even trigger. However I think this requires manually going through the instance and all of its MRO for each and every attribute checking if it might be a proper fixture before accessing it. So I took the easy route here.
So this at least seems to be a step in the right direction. But this was a long time ago.
There's a question if this is any backward compatibility concern here. Is it possible in any way to use a property as a fixture? (I'm not familiar with this code yet).
In any case I think it would be to discuss this change in a separate PR so it can be considered properly.
There was a problem hiding this comment.
as a breaking change its ok to drop consideration for properties
as methods are descriptors, we cant drop everything
i would prefer if we could set up better separation for fixture lokup
RonnyPfannschmidt
force-pushed
the
fix-regenddoc
branch
from
048012f
to
c081c0f
Compare
|
@RonnyPfannschmidt should we close this in the spirit of clearing up the queue, to be tackled again in the future? |
|
I plan to sort it out before 6.0 |
RonnyPfannschmidt
force-pushed
the
fix-regenddoc
branch
from
e08193e
to
3ecf6b7
Compare
|
@nicoddemus i tried to add a test, but its looking like i can't trigger a warning message easily, there may be a missing part of warning coverage |
|
@RonnyPfannschmidt this might be stale after the recent regendoc fixes? |
RonnyPfannschmidt
changed the title
|
@The-Compiler could this be something for 7.x? |
|
I'd honestly rather not ressurrect a 2 year old PR (hopefully) so close to the release - seems like a lot of potential to delay things further. |
|
@RonnyPfannschmidt was this superseded by #12781? |
|
Mostly, I intend is to verify and close this week |
this one carries a bug-fix for for the fixture lookup that we might want to add in general
as well as a stacklevel ix for the terminalwriter deprecation
if necessary i can extract
i'd like to add a unittest for the fixture parsing but i haven't approached the details yet
changelogfolder,