-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Add deprecation warning for convertPreferences() #11174
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@le717 Wording looks OK to me. Should we make use of the second param, oncePerCaller, for deprecationWarning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing.
|
@nethip Suggested change made, ready to go. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent, we should change set to PreferencesManager.set(), too.
Otherwise, this looks good and we definitely want this in Release 1.4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
|
Could you also add the |
|
@marcelgerber All changes made. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The closing single quote should come before comma after printing the client string.
Also more importantly, we could have a uniform deprecation message instead of three different on various occasions. I would suggest a simple and uniform:-
"PreferencesManager.<some_deprecated_fn> called with client id <client_id> has been deprecated. Use PreferencesManager.<updated_fn> instead".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated message. Is that better?
|
The messages are still not quite streamlined, but as it's unlikely, for an end user, to get both at the same time, I'll merge anyway. |
Add deprecation warning for convertPreferences()
|
Edit: There are more than one instance where it is being used. |
|
@sprintr Apologies for what's probably a dumb question, but how exactly would that be updated? Resizer is a core (but not core extension) module, but I have never seen a |
For #10412.
@nethip Any suggestions for the message wording?