bpo-35183: Add typical examples to os.path.splitext docs

[jstockwin]

https://bugs.python.org/issue35183

[dlax]

LGTM

[nanjekyejoannah]

Thanks for the examples. I was curious why don't you have a line explaining each example instead of listing examples close to each other?

Follow the last sentence as example, there is a leading explanation to the example. I see that adding examples this way gives better context unless everything you added captures one case, which I don't think.

Also, see the way examples are documented in the method just before this, in .. function:: splitdrive(path).

[jstockwin]

Hi @nanjekyejoannah, thanks for the suggestion - I think that is a good point.

I've just pushed a commit to that effect, although I found it quite hard to write anything for the preceding context as the examples are quite simple. It'll also take up more room on the page for something reasonably basic, I don't know if that's something we tend to worry about? Despite this, I do think I agree with you and prefer the new version.

A question: Do you think it's fine to refer to what will become ext as "the extension" as I now do in the first two examples. It's fairly obvious that that is what ext is supposed to represent, but it's not explicitly mentioned in the first part of the description.

Finally, this my first contribution to cpython (:tada:), do I need to rebase to squash the commits together, or will the core-dev handle any squashing if/when merging? I've also realised my first commit message (and PR title) should read os.path.splitext, should I force push to fix this? (Note the news entry is correct).

[jdevries3133]

@jstockwin great work! Congrats on your (almost) first contribution to cpython!

[jdevries3133]

This is nitpicky (sorry). Note that up here in the prior docs, "ext" is italicized, then you switch to monospace. In general, throughout python documentation, you'll see italics used when referencing arguments or variable names in the midst of prose. I suggest changing "ext" on lines 490 and 493 to italics to match the style from the first paragraph.

[jdevries3133]

What you've done absolutely does accurately follow the style and patterns seen in this documentation thus far. I think it would be nice to do repr examples like this, though:

If the path contains no extension, *ext* will be an empty string::

    >>> splitpath('bar')
    ('bar', '')

If the path contains an extension, then *ext* will be set to this extension,
including the leading period. Note that previous periods will be ignored::

    >>> splittext('foo.bar.exe')
    ('foo.bar', 'ext')

Leading periods on the basename are also ignored::

    >>> splitext('.cshrc')
    ('.cshrc', '')

BUT

1. That's just me
2. It breaks from the style of other in lined examples throughout this doc

So, I'd recommend just wait and see if / how others react to this comment, and only go ahead and restructure it like this if others agree with me, because might not!

[jdevries3133]

A question: Do you think it's fine to refer to what will become ext as "the extension" as I now do in the first two examples. It's fairly obvious that that is what ext is supposed to represent, but it's not explicitly mentioned in the first part of the description.

I leave the precise wording to you, but I definitely think it's good to explicitly explain what ext is. It can be achieved as simply as adding the full term right before the first introduction of the abbreviation on this line:

- path``, and *ext* is empty or begins with a period and contains at most one
+ path``, and ***the [path?] extension***(*ext*) is empty or begins with a period and contains at most one

Finally, this my first contribution to cpython (🎉), do I need to rebase to squash the commits together, or will the core-dev handle any squashing if/when merging? I've also realised my first commit message (and PR title) should read os.path.splitext, should I force push to fix this? (Note the news entry is correct).

The core dev will squash and cleanup the commit message as they see fit before merging. No need to do any cleanup and definitely do not force push into a PR branch. I believe that it cause the commentary and code reviews to get jarbled or lost all together.

[jstockwin]

@jdevries3133 thanks for your comments. I will change the ext thing, and also modify the wording a bit. Not sure I will get around to this today, though.

Regarding the layout of the examples, I agree it might be nice to do repr examples as you suggest, but also do think it should be consistent. I could change the other examples, but I guess that should come under another issue?

There are also other functions lacking examples. Again, I could go through and add them, but I assume this needs to be different issues.

[nanjekyejoannah]

A question: Do you think it's fine to refer to what will become ext as "the extension" as I now do in the first two examples. It's fairly obvious that that is what ext is supposed to represent, but it's not explicitly mentioned in the first part of the description.

I am not a native english speaker but for now, you can maintain the wording.

Finally, this my first contribution to cpython (🎉), do I need to rebase to squash the commits together, or will the core-dev handle any squashing if/when merging? I've also realised my first commit message (and PR title) should read os.path.splitext, should I force push to fix this? (Note the news entry is correct).

I personally dont care if you dont squash your commits, afterall when I merge PRs, the commits get squashed.

[jdevries3133]

I agree, separate bpo's are the best way forward with those two things. Taking a closer look now, I can see that the only other offender of the e.g. + inlined example pattern is os.splitdrive. It's actually not throughout the doc.

But anyway, just convert ext to italics and this PR is good to go. I think the wording looks great and it's already been reviewed. Of course, you're free to change it if you see issues, but to me it looks fine.

After this PR lands, you or I can feel free to go ahead and create / write patches for additional issues as we see fit.

[jstockwin]

Ok @jdevries3133 (and others), I've pushed 3 new commits. The first fixes ext not being italic. The second is the repr change, and I also modified the docstring of splitdrive, which is the only other offender. The third is a slight wording change.

I don't know if it's a problem that the second new commit (06c0cae) changes splitdrive (so may be out of scope of this bpo). I can always revert the commit if so.

Let me know what you think!

[jdevries3133]

Looks good to me, hopefully this gets reviewed by a core dev soon!

[miss-islington]

Thanks @jstockwin for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.9.
🐍🍒⛏🤖

[bedevere-bot]

GH-27563 is a backport of this pull request to the 3.10 branch.

[bedevere-bot]

GH-27564 is a backport of this pull request to the 3.9 branch.