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.

Sign up for GitHub

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

Jump to bottom

tools: disallow deprecated define getter/setter #6774

Closed
wants to merge 2 commits into from
Closed

tools: disallow deprecated define getter/setter #6774

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented May 15, 2016 •
edited
Loading

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

tools

Description of change

Refs: #6768

This should not land until #6768 lands. Labeling in progress.

/cc @jasnell

@Trott Trott added wip Issues and PRs that are still a work in progress. tools Issues and PRs related to the tools directory. labels May 15, 2016
@Trott Trott mentioned this pull request May 15, 2016
Closed
2 tasks
bnoordhuis
bnoordhuis reviewed May 15, 2016
View reviewed changes
tools/eslint-rules/no-definegetter-definesetter.js
MemberExpression: function(node) {
var prop;
if (node.property) {
if (node.property.type === 'Identifier' && !node.computed) {
Copy link
Member

Choose a reason for hiding this comment

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

!node.computed... so obj['__defineGetter__'] = getter is the escape hatch? =)

@bnoordhuis
Copy link
Member

LGTM. This won't catch { get prop() { ... } } syntax, would it? Not that I think it matters much.

@jasnell
Copy link
Member

jasnell commented May 15, 2016

#6766 is the other PR that would need to land before or along with this. It catches the uses inside internal/process/stdio

@jasnell
Copy link
Member

jasnell commented May 15, 2016

LGTM once #6766 and #6768 land and CI is green.

@jbergstroem
Copy link
Member

LGTM here too.

@jasnell
Copy link
Member

jasnell commented May 17, 2016

Both #6766 and #6768 have landed. The one instance in the tests is still there. @Trott, I'll leave that up to you if you want to change it or ignore it.

Trott added 2 commits May 18, 2016 13:45
@Trott
test: refactor to eliminate __defineGetter__
083a23e 
In preparation for a lint rule to flag `__defineGetter__`, refactor the
one remaining instance in the code base.
@Trott
tools: disallow deprecated define getter/setter
9ea72f0 
Refs: nodejs#6768
@Trott
Copy link
Member Author

Trott commented May 18, 2016

@jasnell I added a commit to handle the one remaining instance in the tests. PTAL

@jasnell
Copy link
Member

jasnell commented May 18, 2016

LGTM if CI is green.

@bnoordhuis
Copy link
Member

Still LGTM FWIW.

@Trott
Copy link
Member Author

Trott commented May 19, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/2704/

Trott added a commit to Trott/io.js that referenced this pull request May 20, 2016
@Trott
test: refactor to eliminate __defineGetter__
37a5a1c 
In preparation for a lint rule to flag `__defineGetter__`, refactor the
one remaining instance in the code base.

PR-URL: nodejs#6774
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request May 20, 2016
@Trott
tools: disallow deprecated define getter/setter
21b53fe 
PR-URL: nodejs#6774
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Refs: nodejs#6768
@Trott
Copy link
Member Author

Trott commented May 20, 2016

Landed in 37a5a1c and 21b53fe

@Trott Trott closed this May 20, 2016
Fishrock123 pushed a commit that referenced this pull request May 23, 2016
@Trott @Fishrock123
test: refactor to eliminate __defineGetter__
0899ccd 
In preparation for a lint rule to flag `__defineGetter__`, refactor the
one remaining instance in the code base.

PR-URL: #6774
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request May 23, 2016
@Trott @Fishrock123
tools: disallow deprecated define getter/setter
eeb0b9e 
PR-URL: #6774
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Refs: #6768
rvagg pushed a commit that referenced this pull request Jun 2, 2016
@Trott @rvagg
test: refactor to eliminate __defineGetter__
c81b6f8 
In preparation for a lint rule to flag `__defineGetter__`, refactor the
one remaining instance in the code base.

PR-URL: #6774
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
@Trott @rvagg
tools: disallow deprecated define getter/setter
ceee56b 
PR-URL: #6774
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Refs: #6768
@MylesBorins MylesBorins mentioned this pull request Jun 2, 2016
Closed
@MylesBorins
Copy link
Contributor

@Trott lts?

@MylesBorins MylesBorins added lts-watch-v4.x and removed wip Issues and PRs that are still a work in progress. labels Jun 3, 2016
@Trott
Copy link
Member Author

Trott commented Jun 3, 2016

@thealphanerd Would need to rewrite the rule to work in v4 so I'm inclined to say no.

@MylesBorins
Copy link
Contributor

don't land it is 😄

@Trott Trott deleted the defgetset branch January 13, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Trott @bnoordhuis @jasnell @jbergstroem @MylesBorins