gh-95011: Migrate syslog module to Argument Clinic by noamcohen97 · Pull Request #95012 · python/cpython · GitHub

This improves code readability and performance by delegating all of the argument parsing logic to Clinic.

Fixes #95011

Issue: Migrate syslog module to Argument Clinic #95011

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

On hold until #95007 is resolved.

Also, please don't force push; it does not play well with the GitHub UI. Please use git merge --no-ff main instead.

Namely, it forces us to reread the whole PR from scratch instead of swiftly looking at a diff:

Looking at the code I found several issues in syslog (#95041). Please wait until I fix them.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@protonmail.com>

I have made the requested changes; please review again.
Still waiting for #95058 to be merged.

Thanks for making the requested changes!

@erlend-aasland: please review the changes made to this pull request.

It looks like Serhiy's comments were addressed. I'm going to land this later today.

Wait a bit more. I think that the new tests are worth to be backported, so I added them to my branch which adds other tests. I worked on this on Saturday, but I need a few more days to let it mature.

Ok, thanks for the heads-up. I'll mark this as don't-merge in the meantime.

Wait a bit more. I think that the new tests are worth to be backported, so I added them to my branch which adds other tests. I worked on this on Saturday, but I need a few more days to let it mature.

@serhiy-storchaka: do you have an issue/PR number for this, so we can track progress?

Also, please don't force push; it does not play well with the GitHub UI. Please use git merge --no-ff main instead.

☝🏻

Sorry for the delay. See #97953. After merging it I'll merge this PR.

#98056 fixed the failing tests

Thank you for your contribution @noamcohen97.

* main: Minor edits to the Descriptor HowTo Guide (pythonGH-24901) Fix link to Lifecycle of a Pull Request in CONTRIBUTING (python#98102) pythonGH-94597: deprecate `SafeChildWatcher`, `FastChildWatcher` and `MultiLoopChildWatcher` child watchers (python#98089) Auto-cancel old builds when new commit pushed to branch (python#98009) pythongh-95011: Migrate syslog module to Argument Clinic (pythonGH-95012)

* main: (5519 commits) Minor edits to the Descriptor HowTo Guide (pythonGH-24901) Fix link to Lifecycle of a Pull Request in CONTRIBUTING (python#98102) pythonGH-94597: deprecate `SafeChildWatcher`, `FastChildWatcher` and `MultiLoopChildWatcher` child watchers (python#98089) Auto-cancel old builds when new commit pushed to branch (python#98009) pythongh-95011: Migrate syslog module to Argument Clinic (pythonGH-95012) pythongh-68686: Retire eptag ptag scripts (python#98064) pythongh-97922: Run the GC only on eval breaker (python#97920) GitHub Workflows security hardening (python#96492) Add `@ezio-melotti` as codeowner for `.github/`. (python#98079) pythongh-97913 Docs: Add walrus operator to the index (python#97921) [doc] Fix broken links to C extensions accelerating stdlib modules (python#96914) pythongh-97822: Fix http.server documentation reference to test() function (python#98027) pythongh-91052: Add PyDict_Unwatch for unwatching a dictionary (python#98055) pythonGH-98023: Change default child watcher to PidfdChildWatcher on supported systems (python#98024) pythonGH-94182: Run the PidfdChildWatcher on the running loop (python#94184) pythongh-92886: make test_ast pass with -O (assertions off) (pythonGH-98058) pythongh-92886: make test_coroutines pass with -O (assertions off) (pythonGH-98060) pythongh-57179: Add note on symlinks for os.walk (python#94799) pythongh-94808: Fix regex on exotic platforms (python#98036) pythongh-90085: Remove vestigial -t and -c timeit options (python#94941) ...

)

bedevere-bot added the awaiting review label Jul 19, 2022

noamcohen97 changed the title Migrate syslog module to Argument Clinic gh-95011: Migrate syslog module to Argument Clinic Jul 19, 2022

arhadthedev reviewed Jul 19, 2022

View reviewed changes

Comment thread Misc/NEWS.d/next/Core and Builtins/2022-07-19-16-32-52.gh-issue-95011.P5t6l9.rst Outdated

noamcohen97 force-pushed the clinic-syslog branch from 3496e2d to 6c6648e Compare July 19, 2022 19:46

noamcohen97 mentioned this pull request Jul 19, 2022

Migrate syslog module to Argument Clinic #95011

Closed

erlend-aasland added the DO-NOT-MERGE label Jul 19, 2022

Migrated syslog module to Argument Clinic

6e2cc36

noamcohen97 force-pushed the clinic-syslog branch from 6c6648e to 6e2cc36 Compare July 20, 2022 05:43

serhiy-storchaka reviewed Jul 20, 2022

View reviewed changes

Comment thread Modules/syslogmodule.c Outdated

Comment thread Modules/syslogmodule.c Outdated

Comment thread Modules/syslogmodule.c Outdated

noamcohen97 and others added 3 commits July 20, 2022 10:24

reformat syslog functions doc

565d4b9

change syslog audit args formatting

8626c65

Merge branch 'main' into clinic-syslog

0585b4d

erlend-aasland requested changes Jul 25, 2022

View reviewed changes

Comment thread Modules/syslogmodule.c Outdated

Comment thread Modules/syslogmodule.c Outdated

Comment thread Modules/syslogmodule.c Outdated

bedevere-bot added awaiting changes and removed awaiting review labels Jul 25, 2022

noamcohen97 and others added 4 commits July 26, 2022 07:49

Update Modules/syslogmodule.c

a2844f0

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@protonmail.com>

Update Modules/syslogmodule.c

9b853db

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@protonmail.com>

syslog: better test coverage

30db659

syslog: fix ref leak

dde4e7c

noamcohen97 commented Jul 26, 2022

View reviewed changes

Comment thread Modules/syslogmodule.c

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 26, 2022

bedevere-bot requested a review from erlend-aasland July 26, 2022 06:09

serhiy-storchaka mentioned this pull request Jul 30, 2022

syslog issues #95041

Closed

kumaraditya303 assigned erlend-aasland Aug 1, 2022

erlend-aasland added the DO-NOT-MERGE label Aug 1, 2022

noamcohen97 added 3 commits August 17, 2022 07:49

Merge remote-tracking branch 'upstream/main' into clinic-syslog

149e646

regen clinic

430d12c

regen global strings

fd6e589

Merge branch 'main' into clinic-syslog

0e0d9c6

noamcohen97 force-pushed the clinic-syslog branch from 7115de2 to 0e0d9c6 Compare September 7, 2022 21:47

serhiy-storchaka mentioned this pull request Oct 6, 2022

Add more syslog tests #97953

Merged

noamcohen97 added 2 commits October 7, 2022 23:21

Merge remote-tracking branch 'upstream/main' into clinic-syslog

f28b332

Merge branch 'main' into clinic-syslog

7a812bc

serhiy-storchaka reviewed Oct 8, 2022

View reviewed changes

Comment thread Lib/test/audit-tests.py Outdated

Comment thread Lib/test/test_audit.py Outdated

remove backported tests

8903e8b

serhiy-storchaka approved these changes Oct 8, 2022

View reviewed changes

Delete 2022-07-19-16-32-52.gh-issue-95011.P5t6l9.rst

e09825c

serhiy-storchaka added skip news and removed DO-NOT-MERGE labels Oct 8, 2022

serhiy-storchaka merged commit 5405537 into python:main Oct 8, 2022

bedevere-bot removed the awaiting merge label Oct 8, 2022

mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022

pythongh-95011 : Migrate syslog module to Argument Clinic (pythonGH-95012

1e204b1

)

gh-95011: Migrate syslog module to Argument Clinic by noamcohen97 · Pull Request #95012 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-95011: Migrate syslog module to Argument Clinic - #95012

gh-95011: Migrate syslog module to Argument Clinic#95012serhiy-storchaka merged 22 commits intopython:mainpython/cpython:mainfrom noamcohen97:clinic-syslognoamcohen97/cpython:clinic-syslogCopy head branch name to clipboard

Conversation

noamcohen97 commented Jul 19, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Jul 19, 2022

Uh oh!

Uh oh!

erlend-aasland commented Jul 19, 2022

Uh oh!

erlend-aasland commented Jul 20, 2022

Uh oh!

arhadthedev commented Jul 20, 2022

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Jul 25, 2022

Uh oh!

Uh oh!

noamcohen97 commented Jul 26, 2022

Uh oh!

bedevere-bot commented Jul 26, 2022

Uh oh!

erlend-aasland commented Aug 1, 2022

Uh oh!

serhiy-storchaka commented Aug 1, 2022

Uh oh!

erlend-aasland commented Aug 1, 2022

Uh oh!

erlend-aasland commented Aug 22, 2022

Uh oh!

erlend-aasland commented Sep 8, 2022

Uh oh!

serhiy-storchaka commented Oct 6, 2022

Uh oh!

noamcohen97 commented Oct 8, 2022

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka commented Oct 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gh-95011: Migrate syslog module to Argument Clinic#95012
serhiy-storchaka merged 22 commits into
python:mainfrom
noamcohen97:clinic-syslog

noamcohen97 commented Jul 19, 2022 •
edited by bedevere-bot
Loading