Skip to content

Add clarifying documentation to Io.links#13791

Open
ElectreAAS wants to merge 1 commit intoocaml:mainfrom
ElectreAAS:push-wllnxnpmvulu
Open

Add clarifying documentation to Io.links#13791
ElectreAAS wants to merge 1 commit intoocaml:mainfrom
ElectreAAS:push-wllnxnpmvulu

Conversation

@ElectreAAS
Copy link
Copy Markdown
Collaborator

If you're thinking of a symlink as an arrow A -> B, the signature val link: src:Path.t -> dst:Path.t -> unit might lead you to believe you should use it like so link ~src:A ~dst:B, but you'd be wrong!
It's the reverse you should be doing.
It threw me off for a loop, so I'm trying to improve the documentation.
Suggestions welcome

Copy link
Copy Markdown
Collaborator

@Sudha247 Sudha247 left a comment

Choose a reason for hiding this comment

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

Good catch!

Comment thread otherlibs/stdune/src/io.mli Outdated
@ElectreAAS
Copy link
Copy Markdown
Collaborator Author

I pushed a new version with revised wording

@Alizter Alizter self-requested a review March 17, 2026 09:48
Comment thread otherlibs/stdune/src/io.mli Outdated
(** Symlink with fallback to copy on systems that don't support it. *)
(** Symlink with fallback to copy on systems that don't support it.

The labels can be misleading:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think saying that the labels can be misleading is a bit strong, because then otherwise why did we choose them?

I think an explanation like the following is enough:

      [portable_symlink ~src ~dst] creates [dst] as a symbolic link 
      (or copy on Windows) pointing to [src]. *)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think this is the convention held up by GNU ln and other tools, but that doesn't mean they're not weird

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As an additional data point, I've heard this complaint a few times but ln is positional, in ln <from> <to> order which matches cp and nobody complains that cp is confusing.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I agree that cp isn't confusing, it works like you'd expect. What I didn't expect was for ln to work like a copy, where I'd figured it would work like an arrow.

I've pushed revised wording

Comment thread otherlibs/stdune/src/io.mli Outdated
Comment thread otherlibs/stdune/src/io.mli Outdated
@ElectreAAS
Copy link
Copy Markdown
Collaborator Author

Pushed revised wording, and rebased on main for good measure

Comment thread otherlibs/stdune/src/io.mli Outdated
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
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