Skip to content

Conversation

Mantisus
Copy link
Collaborator

@Mantisus Mantisus commented Feb 13, 2025

Description

  • Change the cookie storage in the Session from dict to SessionCookies which uses CookieJar. It supports basic cookie parameters and multiple domains.

Issues

Testing

  • Update old tests
  • Add tests for new class
  • Add tests for multidomain support

@Mantisus Mantisus self-assigned this Feb 13, 2025
@Mantisus Mantisus requested review from vdusek and Pijukatel February 13, 2025 12:51
@Mantisus Mantisus marked this pull request as ready for review February 13, 2025 12:51
Copy link
Collaborator

@vdusek vdusek left a comment

Choose a reason for hiding this comment

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

Since it contains a breaking change, could you please describe the breaking changes in the PR's description and also summarize it in the Upgrading guide?

@Mantisus Mantisus requested review from Pijukatel and vdusek February 20, 2025 11:32
Copy link
Collaborator

@vdusek vdusek left a comment

Choose a reason for hiding this comment

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

There are failing tests for Windows Python 3.12.

@Mantisus Mantisus force-pushed the session-with-cookiejar branch from 003faf8 to f136bcd Compare February 20, 2025 23:22
@Mantisus
Copy link
Collaborator Author

There are failing tests for Windows Python 3.12.

The problem is not related to this PR.

It's solved in the PR #1007

Pijukatel
Pijukatel previously approved these changes Feb 21, 2025
@Mantisus Mantisus requested a review from vdusek February 21, 2025 13:46
Copy link
Collaborator

@vdusek vdusek left a comment

Choose a reason for hiding this comment

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

Looks good, just a few minor comments and one concern regarding the usage of HTTPX's cookies.

@Mantisus Mantisus requested a review from vdusek February 25, 2025 01:35
@Pijukatel Pijukatel dismissed their stale review February 25, 2025 08:02

New commits with new comments

@Pijukatel Pijukatel self-requested a review February 25, 2025 10:50
Copy link
Collaborator

@vdusek vdusek left a comment

Choose a reason for hiding this comment

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

LGTM and great job, just wait for the resolution of the last unresolved conversation

@vdusek vdusek merged commit 6523b3a into apify:master Feb 25, 2025
23 checks passed
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.

Standardize cookie handling Add session cookies to crawling context
5 participants