lint: fix deprecation warnings #8423
Conversation
paul-marechal
force-pushed
the
mp/fix-deprecations
branch
2 times, most recently
from
b1d4fd2
to
fd87982
Compare
|
We should remove deprecated APIs which were deprecated before last release. |
|
@akosyakov is there a place where I can see which needs to be removed? |
Build output is cleaner without the verbose option. It was added in order to get better feedback while building, but it turned out to just clutter the terminal. Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
paul-marechal
force-pushed
the
mp/fix-deprecations
branch
3 times, most recently
from
dacc2b2
to
f8d4b16
Compare
|
I searched for
|
paul-marechal
force-pushed
the
mp/fix-deprecations
branch
2 times, most recently
from
45f2db6
to
bade3b4
Compare
Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
paul-marechal
force-pushed
the
mp/fix-deprecations
branch
from
bade3b4
to
3343649
Compare
| @@ -261,6 +255,7 @@ export class KeybindingRegistry { | |||
| containsKeybindingInScope(binding: Keybinding, scope = KeybindingScope.USER): boolean { | |||
| const bindingKeySequence = this.resolveKeybinding(binding); | |||
| const collisions = this.getKeySequenceCollisions(this.keymaps[scope], bindingKeySequence) | |||
| // eslint-disable-next-line deprecation/deprecation | |||
There was a problem hiding this comment.
Why do we need such comments? If there are still usages of deprecated APIs. We still should see it rather than hide.
There was a problem hiding this comment.
It's build noise, and using such comments is actually an act of handling it. If we want to get a list of suppressed deprecations we can still search for deprecation/deprecation. The alternative would be to get rid of the deprecated API indeed, but here I simply had to suppress the warning to say "this is expected".
The line you attached your comment to handles an old API. I didn't know enough about this API to remove it, but if you tell me that we really don't want to keep supporting context then let's remove it.
There was a problem hiding this comment.
I am fine with build noise if it is valid noise, let's remove comments to see usage of deprecated APIs.
There was a problem hiding this comment.
Alternatively we should remove warnings completely from the build, but make sure that deprecation usages are highlighted as errors in the editor.
There was a problem hiding this comment.
The warnings should help when writing new code and having to decide what to do with said warning: Change the code or suppress because we must handle the deprecated but not yet removed API?
It is possible to remove the eslint-disable comments and only have the warnings show in the editor, but it adds complexity to the repo setup. And I think it is much easier to do like what is done in this PR, without losing anything.
There was a problem hiding this comment.
Regarding the code you highlighted, is it safe to remove this context attribute and only keep when on keybindings?
|
Need to find a better way to do this later. |
Signed-off-by: Paul Maréchal paul.marechal@ericsson.com
I went with the shotgun approach where I followed each warning as well as its suggested fix and tried to apply it everywhere possible. Some places I wasn't so sure if the modification would impact behavior, so I left it as is.
A lot of the changes had to do with deprecation warnings from mocha, about strict equality checks.
Fixes #8422
This PR has 2 commits:
How to test
yarnshould build and lint without warnings.yarn testshould pass.Review checklist
Reminder for reviewers