Skip to content

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

7x11x13
Copy link
Contributor

@7x11x13 7x11x13 commented Jul 15, 2024

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 with validate_filename("a" * 256).

I also fixed the test_normal_space_or_period_at_tail test for the Windows and universal platforms.

else:
self._max_len = max_len
self._max_filepath_len = min(max_filepath_len, platform_max_filepath_len)
Copy link
Contributor Author

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.

Copy link
Owner

@thombashi thombashi left a 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?

@7x11x13
Copy link
Contributor Author

7x11x13 commented Jul 28, 2024

I feel that modifying _base.py and _filename.py is unnecessary for this feature. What is the purpose of modifying these files?

For _base.py, currently the default value for max_len (if it is set to None) is set based on _get_default_max_path_len, so I changed the base class to consider both max path len and max file len so the appropriate default could be set. This makes the API for filename and filepath methods more consistent, as currently you cannot set max_len=None for filename methods as you can with filepath methods, otherwise it will give a default value of platform max path length, not max name length.

For _filename.py, it is a similar concern of making the filename and filepath methods have consistently named parameters.

@thombashi
Copy link
Owner

Thank you for your answer.

The classes defined in _base.py are abstract base classes for both filename and filepath.
It is not desirable to define concrete parameters to those base classes such as max_filename_len, etc.
It would destroy the abstraction, and child classes would never need both parameters. It also makes the processing more complicated.
You may have a different opinion, but I feel that the current namings are consistent enough.

So, I believe there is no need to modify _base.py and _filename.py.

@7x11x13
Copy link
Contributor Author

7x11x13 commented Jul 29, 2024

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?

@thombashi
Copy link
Owner

I'm afraid I have to disagree.
max_len is a field that exists in both filename and filepath.
Also, as far as I know, platforms only define the maximum path length, not the maximum filename.
Therefore, the default max_len is expected to be the same for filename and filepath.

@lordwelch
Copy link

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())

@thombashi
Copy link
Owner

I intend that the maximum value of filename length may differ for each system, and there is no common defined value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants