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

New-style classes for python2.7 #2147

Closed
nolar opened this issue Dec 20, 2016 · 14 comments
Closed

New-style classes for python2.7 #2147

nolar opened this issue Dec 20, 2016 · 14 comments
Labels
good first issue easy issue that is friendly to new contributor type: backward compatibility might present some backward compatibility issues which should be carefully noted in the changelog type: feature-branch new feature or API change, should be merged into features branch type: removal marks the actual removal of a feature, usually done in major releases
Milestone

Comments

@nolar
Copy link

nolar commented Dec 20, 2016

The problem:

Pytest uses old-style classes everywhere. E.g.,

class MarkGenerator:
    ………

class MarkDecorator:
    ………

This causes unexpected behaviour in some cases. For example, when trying to get an item from a mark, AttributeError is raised (as in the old-style classes) instead of TypeError (as in the new-style classes).

The old-style classes are long deprecated, and the modern libraries & young developers do not expect this weird behaviour in their code anymore. This causes errors when trying to integrate pytest with other libraries & frameworks. For instance, in Jinja template engine (pallets/jinja#631); but there may be other cases.

The solution:

It would be nice if all classes would be new-style classes, i.e. explicitly inheriting from object:

class MarkGenerator(object):
    ………

class MarkDecorator(object):
    ………

Possible problems:

This will probably break the compatibility with python < 2.7 (officially 2.6+ is declared for pytest).

Also, in python3, the no-braces syntax is a new normal (again), but causes the new-style classes with implicit inheritance from object. The 2.7's new-style syntax is supported.

So, explicit mentioning of (object) may be of use for python2.7 only. Hence, it should be discussed is it worth changing the code for this case.

Notes:

Python 2.7.10.
pytest==3.0.3

@nolar nolar changed the title New-style classes New-style classes for python2.7 Dec 20, 2016
@The-Compiler
Copy link
Member

Why would it break 2.6 compatibility?

@RonnyPfannschmidt
Copy link
Member

we would mainly break behaiviour compatibility, i propose we wed this out while introducing attrs

@nicoddemus
Copy link
Member

New style classes have been introduced a long time ago; I remember using new-style classes was the recommendation for all objects at the time when I started learning Python, and that was in Python 2.4. This document says new-style classes were introduced in 2.1, and object() was introduced in 2.2 and already mentions the object type and new-style classes.

New-style classes affect mainly very special use-cases (like __slots__ and __getattr__), and exceptions raised by special methods like you said:

Python 2.6.6 (r266:84297, Aug 24 2010, 18:46:32) [MSC v.1500 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> class Old: pass
...
>>> class New(object): pass
...
>>> Old()[0]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: Old instance has no attribute '__getitem__'
>>> New()[0]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'New' object does not support indexing

But I don't know currently how it would break compatibility with 2.6... @nolar do you have any exmaples or references on how changing that would break things in 2.6 but not in 2.7?

Having said that, I'm not opposed to the idea at all, it will make the behavior in Python 2 and 3 more consistent; I never worried about this before because the differences are very subtle and I didn't think they mattered on how pytest uses its objects.

If we decide to change this, it must be done in the features branch and mentioned on the CHANGELOG, of course.

@nicoddemus
Copy link
Member

@RonnyPfannschmidt

we would mainly break behaiviour compatibility, i propose we wed this out while introducing attrs

I'm not sure we need to tie one change to the other. Changing to new-style classes is just a matter of a simple find/replace and checking if all tests pass. 😁

@nolar
Copy link
Author

nolar commented Dec 22, 2016

Sorry, that was my mistake. I've misunderstood the sentence "New-style classes has been integrated into Python 2.7 and old-style classes has been removed in Python 3" at https://www.python.org/doc/newstyle/.

If the new-style classes are there since 2.2, then backward compatibility will not be broken.

@nicoddemus
Copy link
Member

@nolar would you like to contribute a PR?

@nicoddemus nicoddemus added good first issue easy issue that is friendly to new contributor type: feature-branch new feature or API change, should be merged into features branch labels Dec 26, 2016
@MichalTHEDUDE
Copy link
Contributor

Hi Guys! Have one question about inheritance from object class.
I saw Issue #2179 and does it cover all classes?
Because I can spot that in pytest/_pytest/vendored_packages/pluggy.py::_TagTracer in row 170 (feature branch) there is class without inheritance from object class.

Should It be also changed? Or there is a 'special' logic behind this?

Thanks!

@RonnyPfannschmidt
Copy link
Member

@MichalTHEDUDE we dont patch vendored code, a pr to pluggy is welcome tho

@MichalTHEDUDE
Copy link
Contributor

Okay, so I can cover all of discrepancies.
But to be clear. Should I add (object) into those folders?

  • doc/
  • testing/

IMO in terms of doc/ should be as is. But what about classes in testing/ folder?

@goodboy
Copy link

goodboy commented Feb 16, 2017

@RonnyPfannschmidt @MichalTHEDUDE made that issue for you.
I don't see any reason this can't be done this week :)

@The-Compiler
Copy link
Member

@MichalTHEDUDE IMHO, it makes sense to do it for all classes in docs and tests, except all TestFoo classes (because pytest only uses classes for grouping, so it really won't make any difference there).

@MichalTHEDUDE
Copy link
Contributor

@tgoodlet so the same changes in pluggy? Coming right up!

@The-Compiler I will make it for everything. And about TestFoo IMO it will be better to also make those changes to be consistent with everything :).
Because in couple of months from now someone will ask, why TestFoo do not inherit from object class??

What do you think?

PS. I also don't see any reasons this can't be done this week ;).

@nicoddemus
Copy link
Member

Because in couple of months from now someone will ask, why TestFoo do not inherit from object class??

Good point!

@nicoddemus
Copy link
Member

Unfortunately we noticed that this might break user's expectations (#2398), so we are backing off this change for now until we can properly follow our deprecation policy in order to introduce this again.

@nicoddemus nicoddemus reopened this May 19, 2017
nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 24, 2017
@nicoddemus nicoddemus added this to the 3.4 milestone Jan 5, 2018
@nicoddemus nicoddemus added the type: backward compatibility might present some backward compatibility issues which should be carefully noted in the changelog label Jan 5, 2018
@nicoddemus nicoddemus added the type: removal marks the actual removal of a feature, usually done in major releases label Jan 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue easy issue that is friendly to new contributor type: backward compatibility might present some backward compatibility issues which should be carefully noted in the changelog type: feature-branch new feature or API change, should be merged into features branch type: removal marks the actual removal of a feature, usually done in major releases
Projects
None yet
Development

No branches or pull requests

6 participants