Skip to content

Add comment explaining that Fpath.follow_symlink is recursive, delete follow_symlinks#14091

Merged
Leonidas-from-XIV merged 1 commit intoocaml:mainfrom
ElectreAAS:comment-about-symlink-rec
Apr 15, 2026
Merged

Add comment explaining that Fpath.follow_symlink is recursive, delete follow_symlinks#14091
Leonidas-from-XIV merged 1 commit intoocaml:mainfrom
ElectreAAS:comment-about-symlink-rec

Conversation

@ElectreAAS
Copy link
Copy Markdown
Collaborator

@ElectreAAS ElectreAAS commented Apr 8, 2026

The presence of both follow_symlink and follow_symlinks (with an s) might lead people to believe that the former will only follow a singular link once. That is not the case, the whole thread is pulled.
Since the plural one was completely unused, I removed it entirely.

Copy link
Copy Markdown
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

This is very nice to know. Could you maybe also add a note in the documentation how these two functions differ, beyond just the return type? Because I share your initial confusion.

@Alizter
Copy link
Copy Markdown
Collaborator

Alizter commented Apr 9, 2026

follow_symlinks appears to be entirely unused...

@Alizter
Copy link
Copy Markdown
Collaborator

Alizter commented Apr 9, 2026

I think you should just get rid of Fpath.follow_symlinks it is dead code, and improve the comment description of Fpath.follow_symlink.

@Leonidas-from-XIV
Copy link
Copy Markdown
Collaborator

Then even better to remove it, move and adapt the documentation comment.

@ElectreAAS
Copy link
Copy Markdown
Collaborator Author

I am quite surprised to learn that the plural version is unused, it seemed like quite a smart thing to do.
Working on #13792, I was taken aback by the lack of universal "sanitize/canonicalize" path functions, and this one seemed to do part of that job

@shonfeder
Copy link
Copy Markdown
Member

Are you planning to use the plural one? That would be a reason to keep it if so.

@shonfeder
Copy link
Copy Markdown
Member

What is needed to get this PR closed or merged?

@ElectreAAS
Copy link
Copy Markdown
Collaborator Author

I just need to take a second look at it, will do now

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
@ElectreAAS ElectreAAS force-pushed the comment-about-symlink-rec branch from c71ba46 to 5386339 Compare April 15, 2026 10:57
@ElectreAAS ElectreAAS changed the title Add comment explaining that Fpath.follow_symlink is recursive Add comment explaining that Fpath.follow_symlink is recursive, delete follow_symlinks Apr 15, 2026
@Leonidas-from-XIV Leonidas-from-XIV merged commit 2fd5ae0 into ocaml:main Apr 15, 2026
29 checks passed
@ElectreAAS ElectreAAS deleted the comment-about-symlink-rec branch April 16, 2026 10:48
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.

4 participants