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

Don't overwrite name with qualname for slots classes #99

Closed
rdpate opened this issue Sep 24, 2016 · 7 comments
Closed

Don't overwrite name with qualname for slots classes #99

rdpate opened this issue Sep 24, 2016 · 7 comments

Comments

@rdpate
Copy link
Contributor

rdpate commented Sep 24, 2016

For slots classes, attr._make.attributes overwrites name with qualname. This shouldn't happen on Python 3. (Nothing happens on Python 2, as qualname is never set.) I think the correct fix is to change:

if repr_ns is None:
    cls_name = getattr(cls, "__qualname__", cls.__name__)
else:
    cls_name = cls.__name__

cls = type(cls_name, cls.__bases__, cls_dict)

To:

cls = type(cls.__name__, cls.__bases__, cls_dict)

However, this doesn't preserve qualname, and I fear I'm missing something else.

@hynek
Copy link
Member

hynek commented Sep 25, 2016

I’m sure there’s a reason and @Tinche will tell us the reason. :) Seems like there’s some edge case around it. We should definitely add a comment to clear it up.

@Tinche
Copy link
Member

Tinche commented Sep 25, 2016

The reason is probably I messed it up by accident and there was no test. :)

I'll take a look later today.

@rdpate
Copy link
Contributor Author

rdpate commented Sep 25, 2016

Now there is a test, in my second link. :) I tried to run tox and submit this as a pull-request, but tox crashed when it ran pip freeze, which crashed because FreeBSD has a system module named "_sqlite3" (which is used by the stdlib module sqlite3). I later noticed qualname wasn't preserved and figured I had best hold off a PR until I can run tests.

@Tinche
Copy link
Member

Tinche commented Sep 25, 2016

I noticed and stole your test. :)

@rdpate
Copy link
Contributor Author

rdpate commented Sep 25, 2016

rdpate@e64d22d

The previous test has a bug: qualname, of course, includes the test class and function. Also, saving and setting qualname seems to be the only other action required. I did get tox -epy24,py34,py35 to run on another machine, and the only failures were slow data generation for hypothesis. I had expected, for no particular reason, for qualname to be included in cls_dict, but I suppose it is implemented as a slot in type.

@Tinche
Copy link
Member

Tinche commented Sep 25, 2016

Great. How about you submit a pull request and we can switch the discussion to there? We're always eager for new contributors. :)

@rdpate
Copy link
Contributor Author

rdpate commented Sep 25, 2016

#100

@rdpate rdpate closed this as completed Sep 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants