-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Small cleanups. #12899
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
Small cleanups. #12899
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.
Haven't fully reviewed this, but I noticed several problems right away that needs to be addressed first.
Returns `True` if the given writable file-like object requires Unicode | ||
to be written to it. | ||
Returns whether the given writable file-like object requires Unicode to be | ||
written to it. |
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.
Personally, I like the original wording a bit better because it states what is returned, rather than implying a boolean result via the word "whether", which might be confusing for non-english-native readers.
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.
A quick grep shows that there's a lot of places where we're using Return whether
to indicate a boolean return type (though admittedly I may be responsible for quite a few of them, as I also prefer that wording). It's also quite common in the CPython docs (for example).
raise ValueError("filename must be a path") | ||
with cbook.open_file_cm(fname_or_fh, "w", encoding="utf-8") as file: | ||
if not cbook.file_requires_unicode(file): | ||
file = codecs.getwriter("utf-8")(file) |
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.
Can we not clobber file
like this, please?
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.
Also, when reassigning a variable within a context like this, I don't think the close() gets called on the new object because the context manager is holding a reference to the original object.
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.
Clobbering what? file
is not a builtin (... anymore). And I'd say it's a much nicer variable name than fh
...
The fact that the codec writer doesn't get closed doesn't matter (and anyways it will get closed by the GC), what matters is that the file itself gets closed.
import codecs
import contextlib
class filelike: # mocks file
def close(self):
print("closed")
@contextlib.contextmanager
def cm(): # mocks open_file_cm
print("enter")
f = filelike()
yield f
f.close()
print("exit")
with cm() as file:
file = codecs.getwriter("utf-8")(file)
print("done")
prints
enter
closed
exit
done
so the file is indeed closed before the contextmanager is exited.
@WeatherGod you are blocking this. I don't think any great harm will be done if this is closed if you can't come to terms with @anntzer |
I don't mind changing the PR either, if my replies above are not convincing enough... |
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.
With or without further changes this is fine.
lib/matplotlib/image.py
Outdated
|
||
Parameters | ||
---------- | ||
v : bool | ||
v : bool or None | ||
If None, use :rc:`image.resample`. |
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.
It would be nice to give the default rcParams setting:
https://matplotlib.org/devdocs/devel/documenting_mpl.html?highlight=documenting#rcparams
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.
done
Lets give @WeatherGod another day or two to respond. If not, I don't think any irrevocable harm is done by merging.... |
@WeatherGod hopefully you aren't too upset, but going to merge this - I don't think the changes made here are a problem, but if you feel strongly we can revert... |
…899-on-v3.1.x Backport PR #12899 on branch v3.1.x (Small cleanups.)
PR Summary
PR Checklist