Skip to content

Conversation

@allrob23
Copy link
Contributor

@allrob23 allrob23 commented Mar 4, 2025

Description

This PR optimizes the way warning suppression is handled by changing _no_warn_slugs from a list to a set. This improves performance when checking if a warning should be suppressed, reducing lookup complexity from O(n) to O(1) on average. Additionally, using a set ensures uniqueness, preventing duplicate warning slugs from being added.

Changes

  • Replaced list[str] with set[str] for _no_warn_slugs.
  • Updated initialization to use set(self.config.disable_warnings).
  • Replaced .append(slug) with .add(slug) to maintain set behavior.

Why?

  • Improves lookup efficiency.
  • Ensures uniqueness of suppressed warning slugs.
  • Reduces potential performance overhead in cases with many warnings.

self._warn_unimported_source = True
self._warn_preimported_source = check_preimported
self._no_warn_slugs: list[str] = []
self._no_warn_slugs: set[str] = []
Copy link
Contributor

Choose a reason for hiding this comment

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

The initialization is still done with an empty list, should use an empty set instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, updated now, thanks for pointing that out

@nedbat
Copy link
Owner

nedbat commented Mar 4, 2025

Thanks! I'm not sure we can measure the speed increase, but it is better philosophically.

@nedbat nedbat merged commit 8c5a5ff into nedbat:master Mar 4, 2025
37 checks passed
@nedbat
Copy link
Owner

nedbat commented Mar 4, 2025

@allrob23 How should I credit you in the contributors file?

@allrob23
Copy link
Contributor Author

allrob23 commented Mar 5, 2025

This PR is part of an ongoing research project. So may I respond to this question later, when and if our paper is published?

@nedbat
Copy link
Owner

nedbat commented Mar 5, 2025

Now I'm curious about the topic of the paper!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants