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-37609: Add device path support in ntpath splitdrive #14841

Closed
wants to merge 5 commits into from
Closed

bpo-37609: Add device path support in ntpath splitdrive #14841

wants to merge 5 commits into from

Conversation

nsiregar
Copy link
Contributor

@nsiregar nsiregar commented Jul 18, 2019 •
edited by bedevere-bot
Loading

Add device path support in ntpath.splitdrive

https://bugs.python.org/issue37609

@nsiregar nsiregar force-pushed the bpo-37609-support-unc-device-path-ntpath-splitdrive branch from f69464e to 589e045 Compare July 19, 2019 17:28
ZackerySpytz
ZackerySpytz reviewed Jul 27, 2019
View reviewed changes
Misc/NEWS.d/next/Library/2019-07-18-14-39-59.bpo-37609.1ZYNbG.rst Outdated
@@ -0,0 +1 @@
Add support for unc device path in :mod:`ntpath.splitdrive`
Copy link
Contributor

Choose a reason for hiding this comment

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

Using the mod role here is not correct. Please see https://devguide.python.org/documenting/#id4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've updated it to use meth.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be func.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've updated it

@nsiregar nsiregar force-pushed the bpo-37609-support-unc-device-path-ntpath-splitdrive branch from e92813b to eb4e996 Compare August 24, 2019 09:26
@zooba
Copy link
Member

zooba commented Sep 11, 2019

Sorry for not seeing this earlier, I have a few general suggestions:

  • put a leading underscore on the new functions (or else add full documentation for them)
  • use constant strings in the comparisons, rather than sep*2
  • add some tests using backslashes as well, not just forward slashes

eryksun
eryksun reviewed Sep 12, 2019
View reviewed changes
Lib/ntpath.py
return p[:2], p[2:]
return p[:0], p


def is_unc_path(path, sep):
Copy link
Contributor

Choose a reason for hiding this comment

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

These functions are clearly private (e.g. parameterized by sep and colon), so the function names should have a leading underscore.

@nsiregar
Copy link
Contributor Author

working on it

@eryksun
Copy link
Contributor

eryksun commented Sep 14, 2019 •
edited by bedevere-bot
Loading

For an alternate and more complete implementation, see the splitdrive.py file that I attached to bpo-37609.

@eryksun eryksun mentioned this pull request May 20, 2022
Closed
@barneygale
Copy link
Contributor

I managed to fix the issue elsewhere (in #91882), so I'm going to close this PR, but thank you very much for taking a look at it! :)

@barneygale barneygale closed this Feb 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZackerySpytz ZackerySpytz ZackerySpytz left review comments

@eryksun eryksun eryksun left review comments

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

Successfully merging this pull request may close these issues.
9 participants
@nsiregar @zooba @eryksun @barneygale @ZackerySpytz @ambv @the-knights-who-say-ni @ezio-melotti @bedevere-bot