-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
turn RecordedWarning into a namedtuple #2014
turn RecordedWarning into a namedtuple #2014
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than a CHANGELOG message.
Please be descriptive of the change, specially that RecordedWarning
is now a namedtuple so if someone gets tripped by it at least there's a clear warning, something like:
RecordedWarning
objects (returned bypytest.warns
andrecwarn
fixture) are nownamedtuples
so they have a better default string representation when displayed in assertion messages (#number).
self.file = file | ||
self.line = line | ||
|
||
RecordedWarning = namedtuple('RecordedWarning', ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a small side effect here, RecordedWarning
attributes are now read-only.
The docs themselves have this to say about it:
Each recorded warning has the attributes message, category, filename, lineno, file, and line. The category is the class of the warning. The message is the warning itself; calling str(message) will return the actual message of the warning.
IMO your change is good and I doubt there are people writing to the attributes, but I thought I would mention this anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, this also means this should go to features
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you feel this way, perhaps you should just add a __repr__
method then? This would solve the issue and could go into master
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, i just reduced the amount of boilerplate to maintain, i wont add it back to add even more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, no problem
ce66923
to
b3c337d
Compare
Sorry for not merging earlier, completely missed the new commits! |
fixes #2013