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

Add __module__ and __qualname__ to methods, fix __name__ #316

Merged
merged 6 commits into from
Dec 27, 2017

Conversation

hynek
Copy link
Member

@hynek hynek commented Dec 17, 2017

Fixes #309

Does this look good to you @Stewori?

@hynek hynek added this to the 17.4 milestone Dec 17, 2017
@codecov
Copy link

codecov bot commented Dec 17, 2017

Codecov Report

Merging #316 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #316   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         745    756   +11     
  Branches      156    157    +1     
=====================================
+ Hits          745    756   +11
Impacted Files Coverage Δ
src/attr/_make.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc0b437...e251ab2. Read the comment docs.

@Stewori
Copy link

Stewori commented Dec 17, 2017

It looks good and thanks for solving this! However, I don't know enough about attrs to fully assess its workability from reading the diff. I suppose that if the dunders are created like expected (and like the tests ensure), this would allow me to fix Stewori/pytypes#18 (not out of the box though, because of __code__.co_filename, see note 2).

Note 1: In _add_method_dunders you should maybe put access to __qualname__ and __module__ into try/catch, because I'm not sure if there can be circumstances under which these attributes are not properly populated in a class (e.g. C-extensions can be odd). Maybe this is not necessary, but from my experience strange things can always happen in Python. Remember: I once assumed a function would always have a __module__ attribute. I would not want this fix to destabilize attrs under some circumstances. However, I don't know an actual example or if it can happen at all. Decide yourself ;)

Note 2: Consider if also __code__.co_filename might better link to the right module name as this is the only way to find the module for a given code object. However, I suppose this is not strictly required to fix Stewori/pytypes#18, because the profiler has access to the function object and thus to __module__. This is just a "nice to have".

Copy link
Member

@blueyed blueyed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!


return self

def _add_method_dunders(self, meth):
"""
Add __module__ and __qualname__ to a method *method*.

This comment was marked as spam.

@hynek
Copy link
Member Author

hynek commented Dec 18, 2017

I once assumed a function would always have a __module__ attribute.

Excellent point. :D

I wonder, if the class hasn't those attributes, is it better to leave them gone or to set them to None? The later sounds cleaner but like something that might mask subtle bugs.

Is there any precedent? I’ve chosen to leave it off for now. Turns out, testing that behaviour is harder than it sounds. :)

Note 2: Consider if also __code__.co_filename might better link to the right module name as this is the only way to find the module for a given code object. However, I suppose this is not strictly required to fix Stewori/pytypes#18, because the profiler has access to the function object and thus to __module__. This is just a "nice to have".

I’m not sure I 100% follow here. Do you want me to set co_filename or to use it? The generated methods do have it but since they’re not in actual files, it’s set to something like '<attrs generated init 97d170e1550eee4afc0af065b78cda302a97674c>' which is used in linecache to look them up if a user wants to pdb thru.

@Stewori
Copy link

Stewori commented Dec 20, 2017

Regarding co_filename it's okay like it is, if it would clash with another use case otherwise :)
Being able to conclude the module from a code object would be nice, but is not crucial.

@hynek hynek merged commit 9e1f136 into master Dec 27, 2017
@hynek hynek deleted the add-module-qualname branch December 27, 2017 10:54
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

Successfully merging this pull request may close these issues.

None yet

3 participants