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

Rename retrieve_widgets to recover_lost_widgets #1524

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Jul 28, 2021

Trac ticket: https://core.trac.wordpress.org/ticket/53816
Trac ticket: https://core.trac.wordpress.org/ticket/53811


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@adamziel adamziel changed the title Update/update/rename retrieve widgets to recover lost widgets Rename retrieve_widgets to recover_lost_widgets Jul 28, 2021
@draganescu
Copy link

I've seen the comment with "lost" in it, but I feel the recover_lost_widgets name is a bit too nice :) Too epic.
What are we doing, actually? It looks like a synchronization to me so maybe sync_registered_widgets ?

@adamziel
Copy link
Contributor Author

adamziel commented Aug 19, 2021

@draganescu it does a lot of things and it's hard to come up with a good name – I think sync_registered_widgets is not perfect, but it's as close as we can get. All I'm after here is to stop suggesting this function only reads and returns something.

@draganescu
Copy link

Thanks for renaming @adamziel. LGTM now!

@TimothyBJacobs
Copy link
Member

This is kinda teetering on the line of refactoring for the sake of refactoring. If we do this rename, I think we should just soft deprecate the retrieve_widgets function so as not to cause unnecessary code churn that is annoying for downstream developers that support more than the latest version of WordPress to support.

@adamziel
Copy link
Contributor Author

adamziel commented Aug 25, 2021

This is kinda teetering on the line of refactoring for the sake of refactoring.

It has that feeling for sure. The thing is that the retrieve_widgets name confused a bunch of people (me included) into thinking it's a read operation and contributed to introducing a few hard to detect bugs. Even worse, we need to call it in the GET endpoint handler – something I would like to address as one of the next steps here. For the sake of our future selves, I would rather make it obvious that it's a write.

If we do this rename, I think we should just soft deprecate the retrieve_widgets function so as not to cause unnecessary code churn that is annoying for downstream developers that support more than the latest version of WordPress to support.

Soft deprecate as in don't call _deprecated_function and only do @deprecated annotation?

@TimothyBJacobs
Copy link
Member

If we do this rename, I think we should just soft deprecate the retrieve_widgets function so as not to cause unnecessary code churn that is annoying for downstream developers that support more than the latest version of WordPress to support.

Soft deprecate as in don't call _deprecated_function and only do @deprecated annotation?

Yeah, that'd be my thinking.

@adamziel
Copy link
Contributor Author

adamziel commented Aug 27, 2021

@TimothyBJacobs done done in e5ce087 ! Would you confirm it's what you had in mind?

@TimothyBJacobs
Copy link
Member

That looks great to me!

Copy link
Contributor

@hellofromtonya hellofromtonya left a comment

Choose a reason for hiding this comment

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

I agree, the new name is better. Why? It's more clear of the reason this function exists as well as what it will do. That makes it more readable and understandable. I agree this is a good change 👍

  • All instances of retrieve_widgets() have been replaced ✅
  • retrieve_widgets() has been soft deprecated ✅
  • Tests @covers are updated ✅
  • PHPCS passes ✅
  • Tests pass ✅

This PR is ready for commit.

@hellofromtonya
Copy link
Contributor

Merged with changeset https://core.trac.wordpress.org/changeset/51705.

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.

4 participants