-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Separate max_len functionality into max_filepath_len and max_filename_len #52
base: master
Are you sure you want to change the base?
Conversation
else: | ||
self._max_len = max_len | ||
self._max_filepath_len = min(max_filepath_len, platform_max_filepath_len) |
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.
Something I noticed while writing this is that the current implementation effectively ignores any max_len (or max_filepath_len) value which is higher than the platform's max path length (thus putting a hard limit on max_filepath_len of 4096). I think it would be better not to do this, but changing it would be a breaking change.
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.
I feel that modifying _base.py
and _filename.py
is unnecessary for this feature.
What is the purpose of modifying these files?
For For |
Thank you for your answer. The classes defined in So, I believe there is no need to |
In that case, I propose we move the setting of default max_len to the concrete classes, so that max_len is not tied to path length in the abstract class. Do you agree? |
I'm afraid I have to disagree. |
Fix for #51. Currently
max_len
is used for two different things: max file path length and max file name length. This PR addresses this problem. It is mostly backwards compatible, with some slight behavior changes (fixes):e.g.
validate_filepath("a" * 256)
used to not raise an error. Now it will raise an INVALID_LENGTH error, which is consistent withvalidate_filename("a" * 256)
.I also fixed the
test_normal_space_or_period_at_tail
test for the Windows and universal platforms.