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

Refactor apply_successful_verification: dead code #2219

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactor apply_successful_verification: dead code #2219

wants to merge 1 commit into from

Conversation

gasull
Copy link

@gasull gasull commented Mar 29, 2021 •
edited

  • Makes all the code reachable but disabled
  • Renamed method for better readibility
  • Type hint that it returns bool

This change is Reviewable

@gasull
Refactor apply_successful_verification: dead code
33eb5ca 
- Makes all the code reachable but disabled
- Renamed method for better readibility
- Type hint that it returns `bool`
@gasull
Copy link
Author

gasull commented Mar 29, 2021

Related issue: #2213

@gasull gasull mentioned this pull request Apr 1, 2021
Closed
@cculianu
Copy link
Collaborator

cculianu commented Apr 6, 2021

Hmm as with the other one -- I'm wary of actually renaming these methods without first making sure no plugin or anything else on the planet calls them. Hmm.

@gasull
Copy link
Author

gasull commented Apr 6, 2021

Hmm as with the other one -- I'm wary of actually renaming these methods without first making sure no plugin or anything else on the planet calls them. Hmm.

Yes, I agree. But are we planning to sort this out in the future?

Firefox has broken compatibility with extensions many times. And I believe Chrome has too. I'm of the opinion that we can do it too if there are benefits, but it must be done in a major release. So I agree this PR is a no-go for now.

Again, I'd suggest building a plugin architecture, which is of course outside the scope of this PR.

I could close this PR, but maybe you want to keep it open or tag it somehow as a future cleanup to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@gasull @cculianu