Skip to content

Conversation

@tboy1337
Copy link

Summary

Refactors the multiple_domains() method in RequestsCookieJar to fix a logic bug and improve performance by using a set for domain tracking instead of a list.

Changes

  • Fixed bug: The previous implementation incorrectly returned True when encountering duplicate domains. The method now correctly checks for multiple unique domains
  • Performance improvement: Changed from list to set for O(1) domain lookups instead of O(n)
  • Simplified logic: More readable implementation that counts unique domains and checks if count > 1
  • Comprehensive tests: Added 8 new unit tests covering various scenarios:
    • Empty jar
    • Single domain with multiple cookies
    • Multiple unique domains
    • Duplicate domains (should count as single)
    • Mixed duplicates and unique domains
    • Edge cases with empty string domains

Files Changed

  • src/requests/cookies.py: Refactored multiple_domains() method (4 lines modified)
  • tests/test_requests.py: Added 65 lines of comprehensive unit tests

Testing

All new tests pass and verify the correct behavior of the method across various domain scenarios.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Performance improvement
  • Test coverage improvement

demonstrate_bug.py

…r domain tracking and add comprehensive unit tests for various domain scenarios.
Copy link
Contributor

@sigmavirus24 sigmavirus24 left a comment

Choose a reason for hiding this comment

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

More statistical hallucinations of an improvement where there's a degradation. I'm not here to help you train a statistical model

…e_domains method in RequestsCookieJar for clarity and added appropriate tests
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.

2 participants