GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-37609: Add device path support in ntpath splitdrive by nsiregar · Pull Request #14841 · python/cpython

Conversation

@nsiregar

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Add device path support in ntpath.splitdrive

https://bugs.python.org/issue37609

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jul 18, 2019
@bedevere-bot bedevere-bot added the awaiting review label Jul 18, 2019
@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
Comment thread Misc/NEWS.d/next/Library/2019-07-18-14-39-59.bpo-37609.1ZYNbG.rst Outdated

@ZackerySpytz ZackerySpytz Jul 27, 2019

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@nsiregar nsiregar Jul 28, 2019

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ZackerySpytz ZackerySpytz Aug 21, 2019

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It should be func.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@nsiregar nsiregar Aug 24, 2019

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
nsiregar and others added 5 commits August 24, 2019 16:24
@nsiregar
bpo-37609: Add device path support in ntpath splitdrive
6ac0014
@blurb-it @nsiregar
📜🤖 Added by blurb_it.
00d328b
@nsiregar
Update logic when found colon in UNC
c0447ff
@nsiregar
Change how detecting extending UNC Path
1981add
@nsiregar
Update role in Misc message
eb4e996
@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 zooba added the needs backport to 3.8 label Sep 11, 2019
@zooba

zooba commented Sep 11, 2019

Copy link
Copy Markdown
Member

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
eryksun
eryksun reviewed Sep 12, 2019
View reviewed changes
Comment thread Lib/ntpath.py
return p[:0], p


def is_unc_path(path, sep):

@eryksun eryksun Sep 12, 2019

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nsiregar

nsiregar commented Sep 14, 2019

Copy link
Copy Markdown
Contributor Author

working on it
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@eryksun

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ambv ambv removed the needs backport to 3.8 label May 4, 2021
@eryksun eryksun mentioned this pull request May 20, 2022
support "UNC" device paths in ntpath.splitdrive #81790
Closed
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@barneygale

barneygale commented Feb 18, 2023

Copy link
Copy Markdown
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! :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

@eryksun eryksun eryksun left review comments
+1 more reviewer
@ZackerySpytz ZackerySpytz ZackerySpytz left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@nsiregar @zooba @eryksun @barneygale @ZackerySpytz @ambv @the-knights-who-say-ni @ezio-melotti @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.