Skip to content

Conversation

@adityagesh
Copy link

@adityagesh adityagesh commented Dec 11, 2024

Handlers is currently initialized as empty list. The operation most performed is contains. Set performs faster for this and ordering of handler is not a concern. Hence replace the list initialization with set.

Handlers is currently initialized as empty list. The operation most per formed is __contains__. Set performs faster for this and ordering of handler is not a concern. Hence replace the list initialization with set.
@adityagesh adityagesh requested a review from vsajip as a code owner December 11, 2024 14:01
@ghost
Copy link

ghost commented Dec 11, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Dec 11, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@adityagesh adityagesh changed the title lib/logging: Set handlers as set instead of list lib/logging: Initialize handlers as Set instead of List Dec 11, 2024
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Sorry, this is a breaking change. handlers is documented as a list: https://docs.python.org/3/library/logging.html#logging.Logger.handlers

@vsajip vsajip added the pending The issue will be closed if no feedback is provided label Dec 11, 2024
@vsajip
Copy link
Member

vsajip commented Dec 11, 2024

As pointed out, a breaking change.

Set performs faster for this

Did you do any benchmarks with realistic scenarios? Even if a set is faster, how much difference does it really make? I'm minded to close this.

@ZeroIntensity
Copy link
Member

If it's a really significant speedup (which I doubt it is), then in theory we could deprecate handlers for a new attribute, and then just reflect changes to it for compatibility. I'm speculating though, and I'd definitely like to see some benchmarks before doing that.

@adityagesh
Copy link
Author

adityagesh commented Dec 12, 2024

I don't have benchmark at the moment. I'll close this for now and reopen when I have data.

@adityagesh adityagesh closed this Dec 12, 2024
@AA-Turner AA-Turner removed the pending The issue will be closed if no feedback is provided label Apr 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants