-
-
Notifications
You must be signed in to change notification settings - Fork 14
Separate max_len functionality into max_filepath_len and max_filename_len #52
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
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. |
For posix systems definitely specify a maximum filename. And posix systems let you query it https://pubs.opengroup.org/onlinepubs/009696799/functions/fpathconf.html: >>> import os
>>> os.pathconf('/', 'PC_NAME_MAX')
255
>>> os.pathconf('/', 'PC_PATH_MAX')
1024 Windows does let you check the filename max but not the path maximum (which is not always 260) import ctypes
import ctypes.windll
import ctypes.wintypes
from typing import NamedTuple
class win32VolumeInfo(NamedTuple):
volume_name: str
filesystem_name: str
max_component_length: int
filesystem_flags: int
serial: int
def get_volume_info(path=None):
"""
Checks for long path support using GetVolumeInformation.
Returns:
win32VolumeInfo
"""
volume_name = ctypes.create_unicode_buffer(ctypes.wintypes.MAX_PATH)
filesystem_name = ctypes.create_unicode_buffer(ctypes.wintypes.MAX_PATH)
max_component_length = ctypes.c_ulong()
serial = ctypes.c_ulong()
filesystem_flags = ctypes.c_ulong()
result = ctypes.windll.kernel32.GetVolumeInformationW(
path, # Defaults to the root directory of the current drive if None
volume_name,
ctypes.wintypes.MAX_PATH,
ctypes.byref(serial),
ctypes.byref(max_component_length),
ctypes.byref(filesystem_flags),
filesystem_name,
ctypes.wintypes.MAX_PATH
)
if not result:
return None
return win32VolumeInfo(ctypes.wstring_at(volume_name), ctypes.wstring_at(filesystem_name), max_component_length.value, filesystem_flags.value, serial.value)
print(get_volume_info()) |
I intend that the maximum value of filename length may differ for each system, and there is no common defined value. |
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.