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

net: remove unused var self = this from old code #5224

Closed
wants to merge 1 commit into from

Conversation

benjamingr
Copy link
Member

This method has an unused var self = this.

Unless I'm missing something obvious here it's redundant.

@thefourtheye thefourtheye added the net Issues and PRs related to the net subsystem. label Feb 14, 2016
@thefourtheye
Copy link
Contributor

Looks straightforward. LGTM. It was introduced in 2106ef0. So, cc @isaacs

@evanlucas
Copy link
Contributor

LGTM

@Trott
Copy link
Member

Trott commented Feb 14, 2016

@thefourtheye
Copy link
Contributor

This is proposed in #5231 as well

@benjamingr
Copy link
Member Author

Hey, I can't see the CI results. Assuming it passed - anything preventing a merge?

@thefourtheye
Copy link
Contributor

Windows has a compilation failure. Weird cc @nodejs/ci

@orangemocha
Copy link
Contributor

c:\workspace\node-compile-windows\label\win-vs2015\deps\openssl\openssl.targets(28,5): error MSB4175: The task factory "XamlTaskFactory" could not be loaded from the assembly "Microsoft.Build.Tasks.Core". Could not find file 'C:\Users\Administrator\AppData\Local\Temp\1\0nhbawpw.dll'. [c:\workspace\node-compile-windows\label\win-vs2015\deps\openssl\openssl.vcxproj]

cc @joaocgreis could it be an environment issue on that machine? See http://www.lenholgate.com/blog/2015/07/the-real-solution-to-error-msb4175-the-task-factory-codetaskfactory-could-not-be-loaded-from-the-ass.html

@ronkorving
Copy link
Contributor

I'm in favor of #5231 over this PR (sorry), as it tackles more issues which are all good to have. Best not to create a potential merge conflict and just go with that imho.

@benjamingr
Copy link
Member Author

@ronkorving I have no commitment to this code - it is merely a small contribution. To be honest - I'm not sure why it hasn't been merged yet. It doesn't conflict with that one or anything.

@ronkorving
Copy link
Contributor

Ah yeah, if they're equal, no point in letting it linger. Same at this point may be said for that other PR. Wish I could merge :)

Removed an unused `var self = this` that is no longer required.

PR-URL: nodejs#5224
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@benjamingr
Copy link
Member Author

@benjamingr
Copy link
Member Author

Landed in e6bfe04

@benjamingr benjamingr closed this Mar 20, 2016
benjamingr added a commit that referenced this pull request Mar 20, 2016
Removed an unused `var self = this` that is no longer required.

PR-URL: #5224
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Mar 22, 2016
Removed an unused `var self = this` that is no longer required.

PR-URL: #5224
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@MylesBorins
Copy link
Contributor

@jasnell lts?

@jasnell
Copy link
Member

jasnell commented Mar 23, 2016

+1

MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
Removed an unused `var self = this` that is no longer required.

PR-URL: #5224
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
Removed an unused `var self = this` that is no longer required.

PR-URL: #5224
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants