Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Stabilize cookie handling #1008

Merged
merged 5 commits into from
Aug 23, 2023
Merged

Stabilize cookie handling #1008

merged 5 commits into from
Aug 23, 2023

Conversation

inancgumus
Copy link
Member

@inancgumus inancgumus commented Aug 22, 2023 •
edited
Loading

What?

Refactors the browser context cookie API methods and makes them return errors to the mapping layer instead of panicking.

Why?

  • Better maintainability and provide the base work for Cookies: Retrieving and filtering by URLs #6.
  • To increase compliance with the mapping layer, like reducing out-of-band panics and providing controllable testing.
  • To improve the error messages for better compatibility with the rest of the module.

Checklist

  • I have performed a self-review of my code
  • I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

Related: #6 and #1005.

@inancgumus
Move cookie methods closer
0ee1032 
To increase comprehensibility, and to make maintainence easier.
@inancgumus inancgumus force-pushed the refactor/browser-context-cookie-api branch from 9b432c4 to 1c0ad02 Compare August 22, 2023 13:36
@inancgumus inancgumus self-assigned this Aug 22, 2023
@inancgumus inancgumus requested review from ankur22 and ka3de August 22, 2023 13:43
@inancgumus inancgumus mentioned this pull request Aug 22, 2023
Closed
@inancgumus inancgumus changed the title Refactor the browser context cookie api Refactor the browser context cookie API Aug 22, 2023
@inancgumus inancgumus force-pushed the refactor/browser-context-cookie-api branch from 1c0ad02 to 0f0a311 Compare August 22, 2023 13:47
@inancgumus
Refactor addCookies
a787622 
Polishes and optimizes (i.e., make([]..)) the method for better
maintainability. Improves the error messages for a greater
compatilibility with the rest of the module.
@inancgumus
Refactor clearCookies
7ae14a9
@inancgumus
Refactor AddCookies to return error
0a83c65
@inancgumus
Refactor clearCookies to return error
92db064
@inancgumus inancgumus force-pushed the refactor/browser-context-cookie-api branch from 0f0a311 to 92db064 Compare August 22, 2023 13:49
@inancgumus inancgumus changed the title Refactor the browser context cookie API Refactor the browser context cookie API and error messages Aug 22, 2023
ka3de
ka3de approved these changes Aug 23, 2023
View reviewed changes
Copy link
Collaborator

@ka3de ka3de left a comment

Choose a reason for hiding this comment

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

LGTM.

ankur22
ankur22 approved these changes Aug 23, 2023
View reviewed changes
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

LGTM. I like the refactors to change from panic to error 🙂

I left a very minor comment.

common/browser_context.go Show resolved Hide resolved
tests/browser_context_test.go Show resolved Hide resolved
common/browser_context.go Show resolved Hide resolved
@inancgumus inancgumus merged commit 2d61d40 into main Aug 23, 2023
@inancgumus inancgumus deleted the refactor/browser-context-cookie-api branch August 23, 2023 08:58
@inancgumus inancgumus changed the title Refactor the browser context cookie API and error messages Stabilize cookie handling Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ka3de ka3de ka3de approved these changes

@ankur22 ankur22 ankur22 approved these changes

Assignees

@inancgumus inancgumus

Labels
cookies refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@inancgumus @ankur22 @ka3de