Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Add deprecation warning for convertPreferences() #11174

Merged
merged 1 commit into from
Jun 20, 2015
Merged

Add deprecation warning for convertPreferences() #11174

merged 1 commit into from
Jun 20, 2015

Conversation

le717
Copy link
Contributor

@le717 le717 commented May 27, 2015

For #10412.

@nethip Any suggestions for the message wording?

@@ -528,7 +532,7 @@ define(function (require, exports, module) {
* @return {boolean} true if a value was set
*/
function setValueAndSave(id, value, options) {
DeprecationWarning.deprecationWarning("setValueAndSave called for " + id + ". Use set instead.");
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing.

@le717
Copy link
Contributor Author

le717 commented Jun 12, 2015

@nethip Suggested change made, ready to go.

@@ -528,7 +532,7 @@ define(function (require, exports, module) {
* @return {boolean} true if a value was set
*/
function setValueAndSave(id, value, options) {
DeprecationWarning.deprecationWarning("setValueAndSave called for " + id + ". Use set instead.");
DeprecationWarning.deprecationWarning("PreferencesManager.setValueAndSave() called for " + id + ". Use set instead.", true);
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@marcelgerber
Copy link
Contributor

Could you also add the @deprecated JSDoc to getPreferenceStorage?

@le717
Copy link
Contributor Author

le717 commented Jun 14, 2015

@marcelgerber All changes made.

@@ -123,7 +125,7 @@ define(function (require, exports, module) {
// having _doNotCreate set to true, then the caller is using the old preferences model.
if (!_doNotCreate) {
var clientString = typeof clientID === "object" ? clientID.uri : clientID;
DeprecationWarning.deprecationWarning("getPreferenceStorage is called with client ID '" + clientString + ",' use PreferencesManager.definePreference instead.");
DeprecationWarning.deprecationWarning("PreferencesManager.getPreferenceStorage() is called with client ID '" + clientString + ",' use PreferencesManager.definePreference() instead.");
Copy link
Contributor

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".

Copy link
Contributor Author

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?

@marcelgerber marcelgerber added this to the Release 1.4 milestone Jun 15, 2015
@marcelgerber
Copy link
Contributor

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.

marcelgerber added a commit that referenced this pull request Jun 20, 2015
Add deprecation warning for convertPreferences()
@marcelgerber marcelgerber merged commit 1a74ece into adobe:master Jun 20, 2015
@sprintr
Copy link
Contributor

sprintr commented Jul 7, 2015

PreferencesManager.convertPreferences is still being used in the core in src/utils/Resizer.js. I think it should be safe to remove it. _isPanelPreferences should be removed as well.

Edit: There are more than one instance where it is being used.

@le717 le717 deleted the issue-10412 branch July 16, 2015 14:25
@le717
Copy link
Contributor Author

le717 commented Jul 16, 2015

@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 panelState key in the preferences file.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants