Skip to content

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

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
Closed
nsiregar wants to merge 5 commits into from
Closed

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

nsiregar wants to merge 5 commits into from

Conversation

@nsiregar
Copy link
Contributor

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

Add device path support in ntpath.splitdrive

https://bugs.python.org/issue37609

@nsiregarnsiregarforce-pushed the bpo-37609-support-unc-device-path-ntpath-splitdrive branch from f69464e to 589e045CompareJuly 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
Copy link
Contributor

@ZackerySpytzZackerySpytzJul 27, 2019

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
ContributorAuthor

@nsiregarnsiregarJul 28, 2019

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

@ZackerySpytzZackerySpytzAug 21, 2019

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
ContributorAuthor

@nsiregarnsiregarAug 24, 2019

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

@nsiregarnsiregarforce-pushed the bpo-37609-support-unc-device-path-ntpath-splitdrive branch from e92813b to eb4e996CompareAugust 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
returnp[:0], p


defis_unc_path(path, sep):
Copy link
Contributor

@eryksuneryksunSep 12, 2019

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
ContributorAuthor

nsiregar commented Sep 14, 2019

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.

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

barneygale commented Feb 18, 2023

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! :)

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

Reviewers

@eryksuneryksuneryksun left review comments

+1 more reviewer

@ZackerySpytzZackerySpytzZackerySpytz left review comments

Reviewers whose approvals may not affect merge requirements

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