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 — gh-77607: Rephrase documentation of os.path.join() by slateny · Pull Request #98995 · python/cpython

Conversation

@slateny

@slateny slateny commented Nov 2, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

https://docs.python.org/dev/library/os.path.html#os.path.join

  • Issue: [doc] Behavior of os.path.join does not match documentation #77607

Sorry, something went wrong.

Uh oh!

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

All reactions
@slateny
Rephrase os.path.join()
17cda59
@bedevere-bot bedevere-bot mentioned this pull request Nov 2, 2022
[doc] Behavior of os.path.join does not match documentation #77607
Closed
@bedevere-bot bedevere-bot added awaiting review docs Documentation in the Doc dir skip news labels Nov 2, 2022
@slateny slateny mentioned this pull request Nov 2, 2022
bpo-33426: [doc] Behavior of os.path.join does not match documentation #28003
Closed
slateny
slateny commented Nov 2, 2022
View reviewed changes
Comment thread Doc/library/os.path.rst
a component is an absolute path, all previous components are thrown away
concatenation of *path* and any non-empty members of *\*paths* separated
by the platform's directory separator character. If the last member of *\*paths*
is empty, the result will end in the platform's directory separator.

@slateny slateny Nov 2, 2022

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.

Made this part a bit more specific, but can use original suggested wording if wanted

Suggested change
is empty, the result will end in the platform's directory separator.
is empty, the result will end in a directory separator.

Sorry, something went wrong.

Uh oh!

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

All reactions

@serhiy-storchaka serhiy-storchaka Nov 2, 2022

Copy link
Copy Markdown
Member

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.

If the last member of *paths
is empty, the result will end in the platform's directory separator.

It is not true. For example, os.path.join('', '') -> ''. On Windows, os.path.join('c:', '') -> 'c:'.

Sorry, something went wrong.

Uh oh!

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

slateny reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@slateny slateny Nov 3, 2022

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.

That's a good point. Then perhaps instead, is empty, and if *path* is not a root dir or empty, then .... It can also be optionally less specific, and say, ... the result will generally end in the ....

Sorry, something went wrong.

Uh oh!

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

All reactions

@serhiy-storchaka serhiy-storchaka Nov 3, 2022

Copy link
Copy Markdown
Member

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 is problematic, because the term "root dir" needs explanation, especially on Windows.

What if just say that empty members are ignored, except the last one?

Sorry, something went wrong.

Uh oh!

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

All reactions

@slateny slateny Nov 4, 2022

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.

If it needs some explanation, then maybe we give a quick example and phrase it as "... and if path is a root directory (i.e., / or c:) or empty, then ..."

For your second statement, do you mean leaving out the note on platform separators? It seems a bit too noteworthy to leave it out though. It could be phrased differently like,

If the last element of paths is empty, the result will end in the platform's directory separator, unless path is empty or a root directory.

Sorry, something went wrong.

Uh oh!

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

All reactions

@serhiy-storchaka serhiy-storchaka Nov 4, 2022

Copy link
Copy Markdown
Member

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.

I afraid that it would be too verbose and still not clear. It is not good place to introduce a new term, especially if it may conflict with the meaning in other sites.

Sorry, something went wrong.

Uh oh!

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

All reactions

@slateny slateny Nov 5, 2022

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.

I see, you would prefer to keep it a bit vague, like The return value is the concatenation of *path* and any non-empty members of *paths* separated by the platform's directory separator character. Empty members of *paths* are ignored except for the last path. If a component is an absolute path, ...?

Sorry, something went wrong.

Uh oh!

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

All reactions
hauntsaninja
hauntsaninja reviewed Nov 2, 2022
View reviewed changes

@hauntsaninja hauntsaninja left a comment

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.

"end with the [...] separator" reads more naturally to me

Sorry, something went wrong.

Uh oh!

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

slateny reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@iritkatriel iritkatriel changed the title gh-77607: Rephrase os.path.join() gh-77607: Rephrase documentation of os.path.join() Nov 2, 2022
slateny
slateny commented Dec 30, 2022
View reviewed changes
Comment thread Doc/library/os.path.rst
Comment on lines +302 to +303
by the platform's directory separator character. If the last member of *\*paths*
is empty, the result will end in the platform's directory separator.

@slateny slateny Dec 30, 2022

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.

@serhiy-storchaka Revisiting this, here's one alternative wording without introducing new terms:

Suggested change
by the platform's directory separator character. If the last member of *\*paths*
is empty, the result will end in the platform's directory separator.
by the platform's directory separator character. If the last member of *\*paths*
is empty, the result will usually end with the platform's directory separator.
This does not happen in cases for certain values of *path*, such as ``'c:'``
on Windows.

That would keep the idea of the original (but incorrect) note of the result will only end in a separator if the last part is empty. If dropping the note entirely is preferred, then I'll happily defer judgement here and in that case feel free to commit the below suggestion instead:

Suggested change
by the platform's directory separator character. If the last member of *\*paths*
is empty, the result will end in the platform's directory separator.
by the platform's directory separator character. Empty members of *\*paths*
are ignored except for the last path.

Sorry, something went wrong.

Uh oh!

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

All reactions
@hauntsaninja

hauntsaninja commented Jan 29, 2023

Copy link
Copy Markdown
Contributor

There was overlap (and a merge conflict) with #100783. Serhiy's feedback applies to that change as well, so I'll open a new PR to fix.

slateny reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@hauntsaninja hauntsaninja closed this Jan 29, 2023
@slateny
slateny deleted the s/77607 branch January 29, 2023 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments
@hauntsaninja hauntsaninja hauntsaninja left review comments

Uh oh!

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

Assignees

No one assigned

Labels

awaiting review docs Documentation in the Doc dir skip news

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.

4 participants

@slateny @hauntsaninja @serhiy-storchaka @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.