GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-95011: Migrate syslog module to Argument Clinic by noamcohen97 · Pull Request #95012 · python/cpython

Conversation

noamcohen97 commented Jul 19, 2022 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

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

Sorry, something went wrong.

Uh oh!

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

arhadthedev and erlend-aasland reacted with thumbs up emoji

All reactions

👍 2 reactions

bedevere-bot commented Jul 19, 2022

Copy link

Copy Markdown

Most changes to Python require a NEWS entry.

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

All reactions

Sorry, something went wrong.

Uh oh!

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

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 Resolved

Uh oh!

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

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

erlend-aasland commented Jul 19, 2022

Copy link

Copy Markdown

Contributor

On hold until #95007 is resolved.

noamcohen97 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.


          Migrated syslog module to Argument Clinic

6e2cc36

noamcohen97 force-pushed the clinic-syslog branch from 6c6648e to 6e2cc36 Compare

July 20, 2022 05:43

erlend-aasland commented Jul 20, 2022

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

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

arhadthedev commented Jul 20, 2022

Copy link

Copy Markdown

Member

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

serhiy-storchaka 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.

serhiy-storchaka reviewed

Jul 20, 2022

View reviewed changes

serhiy-storchaka left a comment

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.

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

Sorry, something went wrong.

Uh oh!

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

erlend-aasland reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Modules/syslogmodule.c Resolved

Uh oh!

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

Comment thread

Modules/syslogmodule.c Resolved

Uh oh!

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

Comment thread

Modules/syslogmodule.c Resolved

Uh oh!

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

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 Resolved

Uh oh!

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

Comment thread

Modules/syslogmodule.c Resolved

Uh oh!

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

Comment thread

Modules/syslogmodule.c Resolved

Uh oh!

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

bedevere-bot added awaiting changes and removed awaiting review labels

Jul 25, 2022

bedevere-bot commented Jul 25, 2022

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

noamcohen97 and others added 4 commits

July 26, 2022 07:49


          Update Modules/syslogmodule.c

a2844f0

Co-authored-by: Erlend Egeberg Aasland <[email protected]>


          Update Modules/syslogmodule.c

9b853db

Co-authored-by: Erlend Egeberg Aasland <[email protected]>


          syslog: better test coverage

30db659


          syslog: fix ref leak

dde4e7c

noamcohen97 commented

Jul 26, 2022

View reviewed changes

Comment thread

Modules/syslogmodule.c Resolved

Uh oh!

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

noamcohen97 commented Jul 26, 2022

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot added awaiting change review and removed awaiting changes labels

Jul 26, 2022

bedevere-bot commented Jul 26, 2022

Copy link

Copy Markdown

Thanks for making the requested changes!

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

All reactions

Sorry, something went wrong.

Uh oh!

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

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

erlend-aasland commented Aug 1, 2022

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

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

kumaraditya303 assigned erlend-aasland

Aug 1, 2022

serhiy-storchaka commented Aug 1, 2022

Copy link

Copy Markdown

Member

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 and noamcohen97 reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

erlend-aasland commented Aug 1, 2022

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

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

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

erlend-aasland commented Aug 22, 2022

Copy link

Copy Markdown

Contributor

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?

noamcohen97 reacted with rocket emoji

All reactions

🚀 1 reaction

Sorry, something went wrong.

Uh oh!

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


          Merge branch 'main' into clinic-syslog

0e0d9c6

noamcohen97 force-pushed the clinic-syslog branch from 7115de2 to 0e0d9c6 Compare

September 7, 2022 21:47

erlend-aasland commented Sep 8, 2022

Copy link

Copy Markdown

Contributor

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

☝🏻

noamcohen97 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.

serhiy-storchaka mentioned this pull request

Oct 6, 2022

Add more syslog tests #97953

Merged

serhiy-storchaka commented Oct 6, 2022

Copy link

Copy Markdown

Member

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

noamcohen97 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.

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

noamcohen97 commented Oct 8, 2022

Copy link

Copy Markdown

Contributor Author

#98056 fixed the failing tests

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka reviewed

Oct 8, 2022

View reviewed changes

Comment thread

Lib/test/audit-tests.py Resolved

Uh oh!

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

Comment thread

Lib/test/test_audit.py Resolved

Uh oh!

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


          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

serhiy-storchaka commented Oct 8, 2022

Copy link

Copy Markdown

Member

Thank you for your contribution @noamcohen97.

All reactions

Sorry, something went wrong.

Uh oh!

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

carljm added a commit to carljm/cpython that referenced this pull request

Oct 9, 2022


          Merge branch 'main' into typewatch

71d48c1

* 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)

carljm added a commit to carljm/cpython that referenced this pull request

Oct 9, 2022


          Merge branch 'main' into urllibnonet

132c32e

* 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)
  ...

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

Reviewers

arhadthedev

arhadthedev left review comments

erlend-aasland

erlend-aasland approved these changes

serhiy-storchaka

serhiy-storchaka approved these changes

Uh oh!

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

Assignees

erlend-aasland

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Migrate syslog module to Argument Clinic

Uh oh!

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

5 participants

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.

GeezerVison

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

noamcohen97 commented Jul 19, 2022 •

edited by bedevere-bot

Loading

Uh oh!

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