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

noamcohen97 · 2022-07-19T16:31:48Z

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

bedevere-bot · 2022-07-19T16:31:51Z

Most changes to Python require a NEWS entry.

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

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

erlend-aasland · 2022-07-19T20:50:56Z

On hold until #95007 is resolved.

erlend-aasland · 2022-07-20T06:54:49Z

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

arhadthedev · 2022-07-20T06:57:37Z

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

serhiy-storchaka

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

Modules/syslogmodule.c

bedevere-bot · 2022-07-25T09:05:41Z

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>

Modules/syslogmodule.c

noamcohen97 · 2022-07-26T06:09:27Z

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

bedevere-bot · 2022-07-26T06:09:30Z

Thanks for making the requested changes!

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

erlend-aasland · 2022-08-01T11:46:59Z

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

serhiy-storchaka · 2022-08-01T13:31:54Z

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.

erlend-aasland · 2022-08-01T16:55:04Z

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

erlend-aasland · 2022-08-22T07:44:23Z

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?

erlend-aasland · 2022-09-08T08:33:25Z

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

☝🏻

serhiy-storchaka · 2022-10-06T07:44:24Z

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

noamcohen97 · 2022-10-08T05:34:56Z

#98056 fixed the failing tests

Lib/test/audit-tests.py

Lib/test/test_audit.py

serhiy-storchaka · 2022-10-08T18:32:38Z

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 ClinicJul 19, 2022

arhadthedev reviewed Jul 19, 2022
View reviewed changes

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

noamcohen97 force-pushed the clinic-syslog branch from 3496e2d to 6c6648eCompare 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 6e2cc36Compare July 20, 2022 05:43

serhiy-storchaka reviewed Jul 20, 2022
View reviewed changes

Modules/syslogmodule.c Outdated Show resolvedHide resolved
Modules/syslogmodule.c Outdated Show resolvedHide resolved
Modules/syslogmodule.c Outdated Show resolvedHide resolved

noamcohen97and 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

Modules/syslogmodule.c Outdated Show resolvedHide resolved
Modules/syslogmodule.c Outdated Show resolvedHide resolved
Modules/syslogmodule.c Outdated Show resolvedHide resolved

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

noamcohen97and 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

Modules/syslogmodule.cShow resolvedHide resolved

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 0e0d9c6Compare 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

Lib/test/audit-tests.py Outdated Show resolvedHide resolved
Lib/test/test_audit.py Outdated Show resolvedHide resolved

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:mainOct 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
)

Uh oh!

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

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

Uh oh!

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

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