Skip to content
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

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.

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.

2 participants