-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
[BUGFIX release] Update deprecation wording in init. #37
Conversation
The deprecation message has been rephrase to emphasize final users don't need to call `this._super`. Though the deprecation is still there, I hope rewording the message would prevent new users from panicking when receiving the deprecation warning.
'Please call this._super(), addon: `' + options.name + '`' | ||
'The addon `' + options.name + '` is overriding init without calling this._super. ' + | ||
'This behaviour is deprecated. ' + | ||
'This means that addon\'s writer needs to update it calling `this._super()` and release a new version. ' + |
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.
s/writer/author/
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.
Both this and the other deprection should make the same suggestion RE: this._super.init && this._super.init.apply(this, arguments);
(the message here just says this._super
).
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.
I kept the original suggestion. I'll extract the deprecation message to be shown before the if
then. Thank you!
Addressed both suggestions. Thank you! |
LGTM @stefanpenner - r? |
'This means that addon\'s author needs to update it calling `this._super.init && this._super.init.apply(this, arguments)` ' + | ||
'and release a new version. ' + | ||
'Do not worry, ember-cli is working normally.' | ||
); |
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.
why is this only one deprecation message now instead of the two separate ones before?
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 difference between them was how to solve it. Addressing this comment made them the same.
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.
This is too verbose (although I do appreciate the reasoning) so lets keep it short and instead link to shortened URL pointing to UPGRADE.MD or something in this repo.
The deprecation message has been rephrase to emphasize final users don't need to call
this._super
.Though the deprecation is still there, I hope rewording the message would prevent new users from panicking when receiving the deprecation warning.
It is easier to update the message than to follow the other steps I suggested in the issue, so it seems like a feasible first step just in case ember-cli-release cannot be easily fixed.