Skip to content
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

[lint] Remove unused variables from .js files #1300

Merged
merged 1 commit into from
Sep 28, 2022

Conversation

dayo09
Copy link
Contributor

@dayo09 dayo09 commented Sep 26, 2022

This commit removes unused variables from .js files.

ONE-vscode-DCO-1.0-Signed-off-by: Dayoung Lee [email protected]


For #1299
From #1253

@dayo09 dayo09 requested review from a team and seanshpark September 26, 2022 05:53
@dayo09 dayo09 added the 2 approvals 2 approvals required to be merged label Sep 26, 2022
@@ -492,7 +492,7 @@ host.BrowserHost = class {
this._view.setSelection(message);
}

_msgColorTheme(message) {
_msgColorTheme() {
Copy link
Contributor

Choose a reason for hiding this comment

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

caller is this._msgColorTheme(message); please check caller.

@@ -135,7 +135,7 @@ host.BrowserHost = class {
this._msgSelection(message);
break;
case 'colorTheme':
this._msgColorTheme(message);
this._msgColorTheme();
Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, do not remove message

This commit removes unused variables from .js files.

ONE-vscode-DCO-1.0-Signed-off-by: Dayoung Lee <[email protected]>
Copy link
Contributor

@seanshpark seanshpark left a comment

Choose a reason for hiding this comment

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

LGTM thank you!

@@ -492,7 +492,7 @@ host.BrowserHost = class {
this._view.setSelection(message);
}

_msgColorTheme(message) {
_msgColorTheme(_message) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I could not fully understand why not to remove the argument. 😢 As the title of this pr, both message isn't used in the method.
@seanshpark @dayo09 Was there any background to remain the argument as variable but adding underscore in front of the name?

Copy link
Contributor

@seanshpark seanshpark Sep 28, 2022

Choose a reason for hiding this comment

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

because I wanted to preserve initial design of using message argument. currently I don't have time to take another look with other codes it would be appropriate to remove or not.

adding underscore will make ignore the tool used for unused variable as I know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yunjayh Well I think so too. 😅 I just put the author's mind as the top priority. I introduced the underscore-ignoring rules to make a room for it.

Copy link
Contributor

@yunjayh yunjayh left a comment

Choose a reason for hiding this comment

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

LGTM Thank you!!!

@llFreetimell llFreetimell merged commit b681cd9 into Samsung:main Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 approvals 2 approvals required to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants