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

lib: replace legacy uses of __defineGetter__ #6768

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 15, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

crypto, readline, internal

Description of change

Minor clean up. There are still some places in core that use the legacy __defineGetter__ syntax. This updates those.

Minor clean up. There are still some places in core that use
the legacy __defineGetter__ syntax. This updates those.
@jasnell jasnell added crypto Issues and PRs related to the crypto subsystem. readline Issues and PRs related to the built-in readline module. labels May 15, 2016
@nodejs-github-bot nodejs-github-bot added tls Issues and PRs related to the tls subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. labels May 15, 2016
@jasnell
Copy link
Member Author

jasnell commented May 15, 2016

@Trott ... is there an eslint rule that can catch these?

@Trott
Copy link
Member

Trott commented May 15, 2016

@Trott ... is there an eslint rule that can catch these?

Not a pre-existing one, but I've made a custom one: #6774

According to that lint rule, there are two files missed in this PR:

  • test/parallel/test-util-inspect.js
  • lib/internal/process/stdio.js

I can do them as part of that PR or you can add them to this one. Doesn't matter to me either way.

@jasnell
Copy link
Member Author

jasnell commented May 15, 2016

I have a separate pr that covers the stdio ones. I intentionally did not touch the tests.

@jasnell
Copy link
Member Author

jasnell commented May 16, 2016

@Trott ... does this PR LGTY?

@jasnell
Copy link
Member Author

jasnell commented May 16, 2016

@Trott
Copy link
Member

Trott commented May 16, 2016

LGTM

jasnell added a commit that referenced this pull request May 17, 2016
Minor clean up. There are still some places in core that use
the legacy __defineGetter__ syntax. This updates most of those.

PR-URL: #6768
Reviewed-By: Rich Trott <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented May 17, 2016

Landed in f293d0b

@jasnell jasnell closed this May 17, 2016
evanlucas pushed a commit that referenced this pull request May 17, 2016
Minor clean up. There are still some places in core that use
the legacy __defineGetter__ syntax. This updates most of those.

PR-URL: #6768
Reviewed-By: Rich Trott <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request May 18, 2016
Trott added a commit to Trott/io.js that referenced this pull request May 20, 2016
PR-URL: nodejs#6774
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Refs: nodejs#6768
Fishrock123 pushed a commit that referenced this pull request May 23, 2016
PR-URL: #6774
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Refs: #6768
@MylesBorins
Copy link
Contributor

@jasnell lts?

rvagg pushed a commit that referenced this pull request Jun 2, 2016
PR-URL: #6774
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Refs: #6768
@Fishrock123
Copy link
Contributor

@thealphanerd Little value; I wouldn't bother.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. readline Issues and PRs related to the built-in readline module. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants