Skip to content

gh-52597: Add position-only markers for os functions #94735

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.

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

Merged
merged 2 commits into from
Sep 29, 2022

Conversation

slateny
Copy link
Contributor

@slateny slateny commented Jul 11, 2022

#52597

chflags skipped as I couldn't test it due to some AttributeError, but per clinic follow_symlinks shouldn't be keyword-only.


For verification, first to get all functions changed:

git diff -U0 HEAD~ | grep '+\.\.' | cut -d' ' -f3 | cut -d'(' -f1

To show the params in the docs:

echo getgrouplist | xargs -n1 -I = grep 'function:: =(' Doc/library/os.rst | sed -r 's|.. function:: (.*)|\1|g'

To show the params in clinic:

echo getgrouplist | xargs -n1 -I = grep '\"=(' Modules/clinic/posixmodule.c.h | sed -r 's|.(.*).module, (.*)...|\1\2|g' | uniq

And combined:

git diff -U0 HEAD~ | grep '+\.\.' | cut -d' ' -f3 | cut -d'(' -f1 | xargs -n1 -I = sh -c "grep 'function:: =(' Doc/library/os.rst | sed -r 's|.. function:: (.*)|\1|g'; grep '\"=(' Modules/clinic/posixmodule.c.h | sed -r 's|.(.*).module, (.*)...|\1\2|g' | uniq; echo" | uniq -u

The last command shows any differences in positional/keyword markers or param names. On visual inspection, all diffs just seem to be differences in param names so the markers in the doc are correct.

@bedevere-bot bedevere-bot added awaiting review docs Documentation in the Doc dir labels Jul 11, 2022
@slateny
Copy link
Contributor Author

slateny commented Jul 11, 2022

@JelleZijlstra Could you take a peek at this one since you'd looked over the docs for io?

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks! I used this also for fixing a few missing pos-only markers in typeshed: python/typeshed#8283, python/typeshed#8284.

@@ -4538,7 +4538,7 @@ The following functions take a process status code as returned by
:func:`system`, :func:`wait`, or :func:`waitpid` as a parameter. They may be
used to determine the disposition of a process.

.. function:: WCOREDUMP(status)
.. function:: WCOREDUMP(status, /)
Copy link
Member

Choose a reason for hiding this comment

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

Strangely enough, this one is indeed pos-only, but all the other similar ones below it aren't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at

"WCOREDUMP($module, status, /)\n"
"--\n"
"\n"
"Return True if the process returning status was dumped to a core file.");
#define OS_WCOREDUMP_METHODDEF \
{"WCOREDUMP", (PyCFunction)os_WCOREDUMP, METH_VARARGS, os_WCOREDUMP__doc__},
static int
os_WCOREDUMP_impl(PyModuleDef *module, int status);
static PyObject *
os_WCOREDUMP(PyModuleDef *module, PyObject *args)
{
PyObject *return_value = NULL;
int status;
int _return_value;
if (!PyArg_ParseTuple(args,
"i:WCOREDUMP",
&status))
goto exit;
_return_value = os_WCOREDUMP_impl(module, status);
if ((_return_value == -1) && PyErr_Occurred())
goto exit;
return_value = PyBool_FromLong((long)_return_value);
exit:
return return_value;
}
#endif /* defined(HAVE_SYS_WAIT_H) && defined(WCOREDUMP) */
#if defined(HAVE_SYS_WAIT_H) && defined(WIFCONTINUED)
PyDoc_STRVAR(os_WIFCONTINUED__doc__,
"WIFCONTINUED($module, /, status)\n"
when the file was first made the markers were already in place, so maybe there's some reason for it.

Separately, could you check whether you're able to run os.plock(op=None) to verify the keyword can't be used? I gave all the functions a quick run to double-check but like chflags, I'm not able to run plock for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, seeing that the clinic def

PyDoc_STRVAR(os_plock__doc__,
"plock($module, op, /)\n"
"--\n"
"\n"
"Lock program segments into memory.\");");

has the position-only marker, then regardless of whether it can be confirmed, adding the plock change would arguably be strictly better than the current docs. In that case, tracking this particular problem can go into some other issue instead, and I'd reckon that the change can go ahead anyways, if there aren't any other problems with the PR.

@JelleZijlstra JelleZijlstra merged commit c759944 into python:main Sep 29, 2022
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

Sorry @slateny and @JelleZijlstra, I had trouble checking out the 3.11 backport branch.
Please backport using cherry_picker on command line.
cherry_picker c759944f16ed9887a77d5ebd738f17f3892c2476 3.11

@miss-islington
Copy link
Contributor

Sorry, @slateny and @JelleZijlstra, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker c759944f16ed9887a77d5ebd738f17f3892c2476 3.10

@AlexWaygood AlexWaygood added needs backport to 3.11 only security fixes and removed needs backport to 3.11 only security fixes labels Sep 29, 2022
@miss-islington
Copy link
Contributor

Thanks @slateny for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-97643 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Sep 29, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 29, 2022
miss-islington added a commit that referenced this pull request Sep 29, 2022
pablogsal pushed a commit that referenced this pull request Oct 22, 2022
@ZeroIntensity ZeroIntensity removed the needs backport to 3.10 only security fixes label Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants