bpo-37689: add Path.is_relative_to() method#14982
Conversation
aeros
left a comment
aeros
left a comment
There was a problem hiding this comment.
@shihai1991 Thanks for the contribution! I definitely think that this function would be quite helpful.
I did some testing in the interpreter after installing python through your branch:
OS: Arch Linux 5.2.3
Looks good to me! I just had a few minor suggestions:
Misc/NEWS.d/next/Library/2019-07-27-18-00-43.bpo-37689.glEmZi.rst
Outdated
Show resolved
Hide resolved
|
@pitrou Hi, Antoine. pls review this PR, if you have free time, thanks :) |
|
@shihai1991 Btw, after you've made any changes based on suggestions you can use "Resolve conversation" to show that each one has been addressed (or directly commit the suggestion to the PR). Thanks for making the recommended changes (: |
pitrou
changed the title
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Thanks for doing this. Please find some comments below.
Doc/library/pathlib.rst
Outdated
|
|
||
| .. method:: PurePath.is_relative_to(*other) | ||
|
|
||
| Return the boolean result that this path is relative to *other* path. |
There was a problem hiding this comment.
How about Return whether this path is relative to the *other* path?
There was a problem hiding this comment.
I agree, this would be an improvement over my earlier suggestion. Since it's modifying a verb, "Return" in this case, I think it might be slightly more correct to explicitly specify the "or not":
Return whether or not this path is relative to the *other* path.
There was a problem hiding this comment.
ops， i am not a native english speaker, so i have no idea about "whether(or not)" :(
There was a problem hiding this comment.
@shihai1991 That's okay, the distinction is often a pain, even for those of us who are native speakers (:
|
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 |
np, looks I need change my habit(because i am a gerrit user before:) |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @pitrou: please review the changes made to this pull request. |
CuriousLearner
left a comment
CuriousLearner
left a comment
There was a problem hiding this comment.
Overall, this looks good to me. I just have a minor suggestion on renaming the parameter as mentioned inline.
CuriousLearner
left a comment
CuriousLearner
left a comment
There was a problem hiding this comment.
Since other methods already have other parameter, I think we're good to go on this.
LGTM 🌮
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Thanks for the update! Just one additional comment and I think we're good to go.
|
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 |
|
Thanks for making the requested changes! @pitrou: please review the changes made to this pull request. |
|
Thank you @shihai1991 ! |
7 participants
https://bugs.python.org/issue37689