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

Remove unimplemented apis #1264

Merged
merged 9 commits into from
Apr 23, 2024
Merged

Remove unimplemented apis #1264

merged 9 commits into from
Apr 23, 2024

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Apr 10, 2024

What?

Removing the unimplemented API function bodies.

Why?

It's confusing when auditing APIs to have some that do not have any implementation. It's as if they are a priority, which is not the case. We will add the APIs that we think we need and when the community asks for them.

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)

@ankur22 ankur22 force-pushed the remove/unimplemented-apis branch 2 times, most recently from edb80c2 to a53651b Compare April 10, 2024 13:35
Copy link

@allansson allansson left a comment

Choose a reason for hiding this comment

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

I've built and run the extension and it works normally.

It's hard to test that something isn't there, but looking at the code I don't see any methods removed that have an implementation and there would definitely be a build error if some reference was left.

I did however find three other methods that don't have an implementation:

I'm not sure if there's a reason why these should remain, but I just wanted to point it out.

@inancgumus
Copy link
Member

inancgumus commented Apr 16, 2024

👍

setExtraHTTPHeaders and LaunchPersistentContext

It seems safe to remove these ones.

goBack

@ankur22 probably hasn't removed goBack because this test requires it. However, we can easily implement an undocumented method that panics (since the test requires "a method" (like GoBack) that panics). And then we can remove GoBack.

@ankur22 ankur22 force-pushed the remove/unimplemented-apis branch from a53651b to d6409e0 Compare April 22, 2024 10:45
@ankur22
Copy link
Collaborator Author

ankur22 commented Apr 23, 2024

setExtraHTTPHeaders and LaunchPersistentContext

Yep, good spot, I've removed them in 4a2e584.

goBack

As @inancgumus mentioned it was indeed due to the goback method, but I've removed that now and changed the test in f9d2190 to still behave in the same way (apart from creating the browserContext and page first).

@ankur22 ankur22 requested a review from allansson April 23, 2024 08:00
@inancgumus inancgumus self-requested a review April 23, 2024 09:05
Copy link

@allansson allansson left a comment

Choose a reason for hiding this comment

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

👍

@ankur22 ankur22 merged commit bf721fb into main Apr 23, 2024
17 checks passed
@ankur22 ankur22 deleted the remove/unimplemented-apis branch April 23, 2024 09:58
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