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

dgram: scope redeclared variables #4940

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 29, 2016

A few variables in lib/dgram.js are redeclared with var in a scope
where they have already been declared. These instances can be scoped
more narrowly with const, so that's what this change does.

@Trott Trott added the dgram Issues and PRs related to the dgram subsystem / UDP. label Jan 29, 2016
@Trott
Copy link
Member Author

Trott commented Jan 29, 2016

@jasnell
Copy link
Member

jasnell commented Jan 29, 2016

LGTM
lts-watch label applied

@cjihrig
Copy link
Contributor

cjihrig commented Jan 29, 2016

LGTM

A few variables in `lib/dgram.js` are redeclared with `var` in a scope
where they have already been declared. These instances can be scoped
more narrowly with `const`, so that's what this change does.
@Trott Trott force-pushed the dgram-no-redeclare branch from de3c655 to 727d89b Compare January 29, 2016 19:08
Trott added a commit to Trott/io.js that referenced this pull request Jan 31, 2016
A few variables in `lib/dgram.js` are redeclared with `var` in a scope
where they have already been declared. These instances can be scoped
more narrowly with `const`, so that's what this change does.

PR-URL: nodejs#4940
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member Author

Trott commented Jan 31, 2016

Landed in 1800bf4

@Trott Trott closed this Jan 31, 2016
rvagg pushed a commit that referenced this pull request Feb 10, 2016
A few variables in `lib/dgram.js` are redeclared with `var` in a scope
where they have already been declared. These instances can be scoped
more narrowly with `const`, so that's what this change does.

PR-URL: #4940
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

@Trott this commit relies on a semver minor change 137f53c#diff-5c0ca4f44209cc4cc68c4dccadbd4a07

would you be able to manually back port it?

@Trott
Copy link
Member Author

Trott commented Feb 17, 2016

@thealphanerd It applies cleanly for me against de0e293 on the v4.x-staging branch. Or is the problem not that it doesn't apply cleanly but that it breaks once it's applied?

@MylesBorins
Copy link
Contributor

@Trott have you updated the branch? How are you landing it? git am is blowing up for me

https://gist.github.com/6c9069bd7457291af1cb

@Trott
Copy link
Member Author

Trott commented Feb 17, 2016

It's working for me like this:

$ git fetch upstream && git rebase upstream/v4.x-staging && git checkout -b test-for-myles && curl -L https://github.com/nodejs/node/pull/4940.patch | git am
remote: Counting objects: 5, done.
remote: Compressing objects: 100% (1/1), done.
remote: Total 5 (delta 4), reused 5 (delta 4), pack-reused 0
Unpacking objects: 100% (5/5), done.
From github.com:nodejs/node
   a99111a..0242c87  v4.x-staging -> upstream/v4.x-staging
First, rewinding head to replay your work on top of it...
Fast-forwarded v4.x-staging to upstream/v4.x-staging.
Switched to a new branch 'test-for-myles'
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   138    0   138    0     0    269      0 --:--:-- --:--:-- --:--:--   270
  0     0    0  1805    0     0   1776      0 --:--:--  0:00:01 --:--:-- 1762k
Applying: dgram: scope redeclared variables
$ 

Trott added a commit to Trott/io.js that referenced this pull request Feb 18, 2016
A few variables in `lib/dgram.js` are redeclared with `var` in a scope
where they have already been declared. These instances can be scoped
more narrowly with `const`, so that's what this change does.

PR-URL: nodejs#4940
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
A few variables in `lib/dgram.js` are redeclared with `var` in a scope
where they have already been declared. These instances can be scoped
more narrowly with `const`, so that's what this change does.

PR-URL: #4940
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 18, 2016
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
A few variables in `lib/dgram.js` are redeclared with `var` in a scope
where they have already been declared. These instances can be scoped
more narrowly with `const`, so that's what this change does.

PR-URL: #4940
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
A few variables in `lib/dgram.js` are redeclared with `var` in a scope
where they have already been declared. These instances can be scoped
more narrowly with `const`, so that's what this change does.

PR-URL: nodejs#4940
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott Trott deleted the dgram-no-redeclare branch January 13, 2022 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dgram Issues and PRs related to the dgram subsystem / UDP.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants