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.

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

Desobekify BrowserContext option parsing #1510

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

inancgumus
Copy link
Member

@inancgumus inancgumus commented Nov 4, 2024

What?

This is an experimental PR that is open for discussion.

It moves the Sobek-dependent option parsing part out of the business logic. Once we finish doing the same with the rest of the option parsing, we can entirely remove the Sobek dependency from the business logic.

Why?

We currently parse Sobek values into our business logic types, but the parsing dependency is in an unrelated package (common). To enhance maintainability, we move the parsing process and keep related components together (in the mapping layer).

Additionally, we could implement a Validate method for the business types (in the common package) to check their validity separately from the Sobek transformation. We can do this in a later PR when needed.

Checklist

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

Related PR(s)/Issue(s)

Updates #1064

@inancgumus inancgumus added refactor stability runtime stability improvements labels Nov 4, 2024
@inancgumus inancgumus self-assigned this Nov 4, 2024
@inancgumus inancgumus requested a review from ankur22 November 4, 2024 17:03
@inancgumus inancgumus marked this pull request as ready for review November 4, 2024 17:03
ankur22
ankur22 previously approved these changes Nov 5, 2024
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.

This makes sense to me. Moving the parsing into the browser package (mapping layer) means that it is closer to where it is used, removing it from a package where it isn't used. This should make things clearer for new devs too.

@inancgumus inancgumus mentioned this pull request Nov 5, 2024
3 tasks
@inancgumus inancgumus force-pushed the fix/browser-context-geolocation-accuracy branch from 377221b to 2c627af Compare November 5, 2024 17:29
Base automatically changed from fix/browser-context-geolocation-accuracy to main November 5, 2024 17:33
@inancgumus inancgumus dismissed ankur22’s stale review November 5, 2024 17:33

The base branch was changed.

Moves the Sobek-dependent option parsing part out of the business logic.

Once we finish, we can entirely remove the Sobek dependency.
@inancgumus inancgumus force-pushed the desobekify/browser-context-option-parsing branch from 7bd70a7 to 85c37b7 Compare November 5, 2024 17:35
@inancgumus inancgumus merged commit 946e4cb into main Nov 5, 2024
17 of 22 checks passed
@inancgumus inancgumus deleted the desobekify/browser-context-option-parsing branch November 5, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor stability runtime stability improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants