Repository navigation
Conversation
| ["{}.{}".format(default_basename, default_filetype)], | ||
| ("{}-{}.{}".format(default_basename, i, default_filetype) | ||
| for i in itertools.count(1))) | ||
| if not os.path.isfile(os.path.join(save_dir, filename))) |
There was a problem hiding this comment.
neat trick! Combining next() with a generator like that, I'll have to remember that one.
| # Characters to be avoided in a NT path: | ||
| # https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx#naming_conventions | ||
| removed_chars = \ | ||
| {"posix": " /\0", "nt": r'<>:"/\|?*\0'}.get(os.name, "_") |
There was a problem hiding this comment.
Is the 'nt' case missing ' '?
There was a problem hiding this comment.
Yes. This is intentional, because there's literally nothing wrong with spaces in filenames on Windows (for many users, most relevant files live in "C:\Documents and Settings..."). (I would argue that it's also the case on Linux but at least you could say well they interact poorly with badly written shell scripts.)
On the other hand I should have mentioned this in the PR doc, sorry about the omission.
I'm going to self-block the PR until we can agree about that change (or until you can convince me to change it back...). EDIT: Apparently I can't self-review a PR, so just going to mark it as WIP for now :-)
There was a problem hiding this comment.
Even if it is technically ok it is still bad practice (see http://www2.stat.duke.edu/~rcs46/lectures_2015/01-markdown-git/slides/naming-slides/naming-slides.pdf slides from an R lecture, but the point stands). We should not by default be suggesting file names with spaces in them. We should also not suggest different default file names between windows and *nix.
I would not support sanitizing direct input from the user (ex the input to savefig). If users want to shoot them selves in their foot, that is their business, but we should not be shooting their feet for them with the defaults.
There was a problem hiding this comment.
The argument from the slides is
new to regular expressions and globbing? be kind to
yourself and avoid
- spaces in file names
- punctuation
- accented characters
- different files named “foo” and “Foo”
but I don't see how this is even relevant, because globbing occurs after word splitting (and, of course, languages other than shell are not affected at all).
sh-4.4$ mkdir test; echo 'foo' > 'test/with space and àccént & punc;tu:a>t<ion'; cat test/*
foo
sh-4.4$ python -c 'import glob; print(glob.glob("test/*"))'
['test/with space and àccént & punc;tu:a>t<ion']
There was a problem hiding this comment.
@tacaswell if you have a look at #9056 you see that the user does not always want to shoot their own foot. Sometimes it is the interaction of several features, like in that case setting a title will affect the chosen name when you click the save button. And when matplotlib proposes a name which does not follow the naming conventions of an operating system, that is bad in my opinion.
tacaswell
left a comment
tacaswell
left a comment
There was a problem hiding this comment.
Filter out ' 'on nt as well.
dstansby
modified the milestones:
2.2 (next next feature release),
2.1 (next point release)
|
Needs a rebase, but otherwise looks good. |
|
@tacaswell do you want to do it? I don't really like the turn this has taken (see above review discussion), but won't prevent you from merging this of course. |
|
I'd love to see this merged, what happened to this pull request? |
I would actually think that we should not do any sanitization ("you get
what you asked for"), but given that there was some opposition last time
I tried to restore the space in the default filename ("Figure 1.png"),
even though the space is totally innocuous for the filesystem, we may as
well fix the filenames that are *actually* problematic.
This makes the default suggested filename independent of the OS.
39379ea to
715f5da
Compare
tacaswell
changed the title
|
Does this need an entry in API changes? |
|
👍 to an API changes entry. |
|
Closing due to disagreement about handling of spaces. |
|
@tacaswell is there a reason why this did not get merged after the change to include spaces? I have no opinion one way or the other on the inclusion of |
Fixes #9056.
I would actually think that we should not do any sanitization ("you get
what you asked for"), but given that there was some opposition last time
I tried to restore the space in the default filename ("Figure 1.png"),
even though the space is totally innocuous for the filesystem, we may as
well fix the filenames that are actually problematic.
As usual feel free to directly (force-)push minor fixes to the PR.
PR Summary
PR Checklist