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

Add follow_symlinks argument to ArtifactoryPath.is_dir #440

Merged
merged 3 commits into from
Feb 10, 2024

Conversation

zhan9san
Copy link
Contributor

@zhan9san zhan9san commented Feb 6, 2024

artifactory.py Outdated
Comment on lines 1894 to 1895
if follow_symlinks:
raise TypeError('Artifactory does not have symlink feature')
Copy link

@barneygale barneygale Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just ignore the flag IMO.

Suggested change
if follow_symlinks:
raise TypeError('Artifactory does not have symlink feature')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@barneygale

Thanks for your review.

The is_dir will also be exposed to real user. The follow_symlinks is follow the same way how Pathlib does, but it's just a placeholder, and not supported in Artifactory. It may be confused if the end user pass follow_symlinks=True.

The Zen of Python

Errors should never pass silently.

Copy link

@barneygale barneygale Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing follow_symlinks=True means "if you encounter a symlink, follow it". That's not an error, even if the filesystem has no symlinks or doesn't support symlinks.

artifactory.py Outdated Show resolved Hide resolved
@zhan9san zhan9san force-pushed the feature/add-follow-symlinks branch from ecce73b to e4aaf76 Compare February 8, 2024 04:13
@zhan9san
Copy link
Contributor Author

zhan9san commented Feb 8, 2024

@barneygale

I understand your point. To avoid any misunderstanding, please allow me to leave more detailed information here.

follow_symlinks

Python <= 3.11.3, there is no follow_symlinks=False passed to entry.is_dir in _RecursiveWildcardSelector._iterate_directories

Python > 3.11.3, follow_symlinks=False is introduced when we use glob feature.

Let's see follow_symlinks change history in glob feature.

In Python 3.11.4-3.11.7, the follow_symlinks is explicitly set to False in pathlib._RecursiveWildcardSelector._iterate_directories.

In Python 3.12.0-3.12.2, the follow_symlinks is also explicitly set to False, but in pathlib.walk which is invoked in pathlib._RecursiveWildcardSelector._iterate_directories.

In Python latest(2/8/2024) main branch, pathlib is removed, pathlib._abc is introduced, and follow_symlinks=None is set to None in pathlib._abc.glob.

is_dir

In pathlib, it is is_dir in Modules/posixmodule.c is used if we use glob(instance of DirEntry) all the time(~3.11.3 - latest main branch), while is_dir in pathlib/_abc.py is used if glob is not used(instance of Path)

As the default value of follow_symlinks changes, it is better to ignore variable follow_symlinks in is_dir as Artifactory does not use it.

In Artifactory, we use the same is_dir no matter glob is used or not.

In Path class, the glob function is using os.scan, and it's a posix.DirEntry instance not Path instance, so it will use the method in Modules/posixmodule.c instead of pathlib/_abc.py.

In ArtifactoryPath class, there is no specific DirEntry-like class.


Please correct me if I say anything wrong

@barneygale
Copy link

barneygale commented Feb 8, 2024

That seems broadly correct.

In Python latest(2/8/2024) main branch, pathlib is removed, pathlib._abc is introduced, and follow_symlinks=None is set to None in pathlib._abc.glob.

Path.glob(follow_symlinks=None) is legacy behaviour that I'm intending to deprecate in Python 3.15 and remove in Python 3.17. It means "follow symlinks, except when expanding ** wildcards".

I'll change the default value to True in PathBase.glob() shortly.

is_dir() only accepts True and False for follow_symlinks

In ArtifactoryPath class, there is no specific DirEntry-like class.

ArtifactoryPath is itself the DirEntry-like class. If you make ArtifactoryPath inherit from pathlib_abc.PathBase and implement iterdir() and stat() (or is_dir()), globbing should just work.

@allburov allburov merged commit c99d910 into devopshq:master Feb 10, 2024
5 checks passed
@zhan9san zhan9san deleted the feature/add-follow-symlinks branch February 10, 2024 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants