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

Disambiguate symlink argument names #79060

Merged
merged 1 commit into from
Nov 20, 2020
Merged

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Nov 14, 2020

The current argument naming in the following standard library functions is horribly ambiguous.

Notice that Swift uses one of the same names we do (dst) to refer to the opposite thing.


the one that exists the one that is
being created
reference
Rust src dst
Swift withDestinationPath
destPath
atPath
path
https://developer.apple.com/documentation/foundation/filemanager/1411007-createsymboliclink
D original link https://dlang.org/library/std/file/symlink.html
Go oldname newname https://golang.org/pkg/os/#Symlink
C++ target link https://en.cppreference.com/w/cpp/filesystem/create_symlink
POSIX path1 path2 https://pubs.opengroup.org/onlinepubs/9699919799/functions/symlink.html
Linux target linkpath https://man7.org/linux/man-pages/man2/symlink.2.html

Out of these I happen to like D's argument names and am proposing that we adopt them.

@rust-highfive
Copy link
Collaborator

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 14, 2020
@jyn514
Copy link
Member

jyn514 commented Nov 14, 2020

Personally I prefer target over original ('the target of the symbolic link'), especially since original may not exist on disk. +1 to improving this one way or another, though.

@jyn514 jyn514 added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 14, 2020
@dtolnay
Copy link
Member Author

dtolnay commented Nov 14, 2020

"Target" is close to synonymous with "destination" and almost as bad IMO.

dtolnay added a commit to dtolnay/cxx that referenced this pull request Nov 14, 2020
@Mark-Simulacrum
Copy link
Member

r=me implementation wise

I do think that original is better than target (target for what?). I sort of want to do "more" here (e.g. force callers to explicitly name their arguments) but I don't think we can readily do so today and this seems like an improvement already. Feel free to r=me unless you want to let this sit for libs feedback (or just general discussion). It seems low cost to rename these multiple times though if necessary.

@ehuss
Copy link
Contributor

ehuss commented Nov 14, 2020

@mati865
Copy link
Contributor

mati865 commented Nov 15, 2020

Notice that Swift uses one of the same names we do (dst) to refer to the opposite thing.

IMO their names are even worse but it's subjective.

Both original and src for the first argument seem pretty clear but for the second argument link would be big improvement.
You could ask the community to think of the new names, maybe somebody comes up with even better ones.

@the8472
Copy link
Member

the8472 commented Nov 15, 2020

ln uses target and link_name

https://man7.org/linux/man-pages/man1/ln.1.html

@dtolnay
Copy link
Member Author

dtolnay commented Nov 19, 2020

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Nov 19, 2020

📌 Commit 29128a5 has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 19, 2020
@bors
Copy link
Contributor

bors commented Nov 19, 2020

⌛ Testing commit 29128a5 with merge 09c9c9f...

@bors
Copy link
Contributor

bors commented Nov 20, 2020

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 09c9c9f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 20, 2020
@bors bors merged commit 09c9c9f into rust-lang:master Nov 20, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 20, 2020
@dtolnay dtolnay deleted the symlinkarg branch November 23, 2020 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants