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.

Sign up for GitHub

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

Jump to bottom

bpo-39950: add pathlib.Path.hardlink_to() method that supersedes link_to() #18909

Merged
merged 2 commits into from
Apr 23, 2021
Merged

bpo-39950: add pathlib.Path.hardlink_to() method that supersedes link_to() #18909

merged 2 commits into from
Apr 23, 2021

Conversation

barneygale
Copy link
Contributor

@barneygale barneygale commented Mar 11, 2020 •
edited by miss-islington
Loading

The argument order of link_to() is reversed compared to what one may expect, so:

a.link_to(b)

Might be expected to create a as a link to b, in fact it creates b as a link to a, making it function more like a "link from". This doesn't match symlink_to() nor the documentation and doesn't seem to be the original author's intent.

This PR deprecates link_to() and introduces hardlink_to(), which has the same argument order as symlink_to().

https://bugs.python.org/issue39950

Automerge-Triggered-By: GH:brettcannon

@tirkarthi
Copy link
Member

The issue was closed as a duplicate so I would prefer closing the PR. Thanks.

@barneygale barneygale changed the title bpo-39925: add pathlib.Path.hardlink_to() method that supersedes link_to() bpo-39950: add pathlib.Path.hardlink_to() method that supersedes link_to() Mar 13, 2020
@barneygale
Copy link
Contributor Author

I've logged a new bug after some discussion on the python-dev mailing list: https://bugs.python.org/issue39950

brettcannon
brettcannon requested changes Mar 13, 2020
View reviewed changes
Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Some doc restructuring and also making sure the public API doesn't break due to this.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@brettcannon brettcannon requested a review from pitrou March 13, 2020 19:07
@barneygale barneygale force-pushed the bpo-39925-pathlib-path-hard-link-arg-order branch from 248c6e8 to 430b7f7 Compare March 21, 2020 02:09
pitrou
pitrou reviewed Apr 17, 2020
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

This looks mostly good. Just two comments.

Also, can you rebase/merge from master?

@barneygale barneygale force-pushed the bpo-39925-pathlib-path-hard-link-arg-order branch from b32aaa6 to b7c8ab2 Compare May 29, 2020 01:27
@barneygale barneygale force-pushed the bpo-39925-pathlib-path-hard-link-arg-order branch 2 times, most recently from ec1bea4 to a65799e Compare January 22, 2021 00:41
@barneygale barneygale changed the title bpo-39950: add pathlib.Path.hardlink_to() method that supersedes link_to() WIP: bpo-39950: add pathlib.Path.hardlink_to() method that supersedes link_to() Jan 22, 2021
@barneygale
Copy link
Contributor Author

Marking as WIP as I'll address bpo-42999 first.

@barneygale barneygale force-pushed the bpo-39925-pathlib-path-hard-link-arg-order branch 2 times, most recently from de0b686 to 2dfa391 Compare April 7, 2021 17:02
@barneygale barneygale changed the title WIP: bpo-39950: add pathlib.Path.hardlink_to() method that supersedes link_to() bpo-39950: add pathlib.Path.hardlink_to() method that supersedes link_to() Apr 7, 2021
@barneygale
Copy link
Contributor Author

Removed 'WIP' from the title. This is ready to review again! Thanks all.

@brettcannon
Copy link
Member

@barneygale please see #18909 (comment) on how to ask for a new round of reviews.

@barneygale
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from brettcannon April 7, 2021 21:56
@brettcannon
Copy link
Member

LGTM!

@barneygale are you up for writing the "What's New" entry to document the addition of the hardlink_to() and the deprecation of link_to()? It's okay if you're not, but it simplifies things for the release if it's updated now.

@barneygale
Copy link
Contributor Author

LGTM!

@barneygale are you up for writing the "What's New" entry to document the addition of the hardlink_to() and the deprecation of link_to()? It's okay if you're not, but it simplifies things for the release if it's updated now.

I've added a new commit - hope it looks alright?

@barneygale
bpo-39925: add pathlib.Path.hardlink_to() method that supersedes `l…
e323969 
…ink_to()`

The argument order of `link_to()` is reversed, so:

    a.link_to(b)

Might be expected to create *a* as a link to *b*, in fact it creates *b*
as a link to *a*. This doesn't match `symlink_to()` nor the documentation
and doesn't seem to be the original author's intent.

This commit deprecates `link_to()` and introduces `hardlink_to()`, which
has the same argument order as `symlink_to()`.
@barneygale
Add info to "What's New" document
e506cae
@barneygale barneygale force-pushed the bpo-39925-pathlib-path-hard-link-arg-order branch from 94c5acb to e506cae Compare April 13, 2021 02:00
@barneygale barneygale requested a review from pitrou April 13, 2021 10:44
@barneygale
Copy link
Contributor Author

@brettcannon sorry to bother, just bumping this review as I saw there's a feature freeze in two weeks! Thanks.

@brettcannon
Copy link
Member

@barneygale yep, this is the top of my review queue and I'm hoping to get to it this Friday (steering council stuff has to take priority, unfortunately).

@miss-islington miss-islington merged commit f24e2e5 into python:master Apr 23, 2021
@brettcannon
Copy link
Member

@barneygale Thanks!

@barneygale
Copy link
Contributor Author

Thanks so much for the review! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brettcannon brettcannon brettcannon approved these changes

@pitrou pitrou Awaiting requested review from pitrou

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@barneygale @tirkarthi @bedevere-bot @brettcannon @pitrou @the-knights-who-say-ni @miss-islington