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

PreferencesManager.convertPreferences() does not throw warning #10412

Closed
le717 opened this issue Jan 19, 2015 · 7 comments
Closed

PreferencesManager.convertPreferences() does not throw warning #10412

le717 opened this issue Jan 19, 2015 · 7 comments

Comments

@le717
Copy link
Contributor

le717 commented Jan 19, 2015

This was brought up by an extension author I was talking to some time back, but I forgot to report this.

It's known the old preferences system is deprecated and will be removed (I already have a PR up to remove it). The PreferencesManager.convertPreferences() method, used to convert the preferences to the new system, will obviously be removed at the same time. However, as the author mentioned to me, the method throws no error saying it will be removed in the future. In fact, it works silently, and the deprecation warnings only fire if the old get/set methods are directly accessed, not when preferences are converted.

The author thought convertPreferences() should be throwing a deprecation warning to alert that it too will soon go away, and with a select few extensions still using the old/conversion system, I think it may be a good idea to quickly add one so it will go off in the v1.2 release, which will could allow us to remove the old system as early as 1.3 or 1.4.

(Side note: If you are reading this and are the extension author I spoke to, please speak up. I have completely forgot who I spoke to 😢).

@marcelgerber
Copy link
Contributor

It was @gruehle over at gruehle/MarkdownPreview@8450de4#commitcomment-9051689.

But I just did a quick search for convertPreferences in the whole registry and there wasn't a single result, so I suppose we can savely remove convertPreferences in 1.2 already.

@peterflynn
Copy link
Member

If there are still authors using the old preferences API, then convertPreferences() is important to keep for a while so they have a clear path for migrating to the new API when the old one is removed.

@marcelgerber
Copy link
Contributor

Yeah, there are still multiple such extensions around, but unfortunately, most extension authors simply ignore the deprecation warnings (see #8751 (comment)) posted to their extensions repo.

@le717
Copy link
Contributor Author

le717 commented Jan 21, 2015

If there are still authors using the old preferences API, then convertPreferences() is important to keep for a while so they have a clear path for migrating to the new API when the old one is removed.

Yeah, there are still multiple such extensions around, but unfortunately, most extension authors simply ignore the deprecation warnings... posted to their extensions repo.

This is why I'm thinking a deprecation warning added should be added even late in v1.2. Maybe the warnings will be enough to trigger authors to remove the call and reliance on the old preferences system. Authors have had a really long time to transition. This would be "the last chance" before removal. It's not as if they were not warned.

@le717
Copy link
Contributor Author

le717 commented Feb 15, 2015

Now that we've reached v1.3, could we soon reach a decision on either issuing a "last call" warning or going ahead and removing the old pref system?

@nethip
Copy link
Contributor

nethip commented May 27, 2015

@le717 Let us target adding a deprecation warning for 1.4. Will you be able to put up a PR for that?

@le717
Copy link
Contributor Author

le717 commented May 27, 2015

@nethip Certainly.

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

No branches or pull requests

5 participants
@peterflynn @nethip @marcelgerber @le717 and others