New: Accessibility Modifiers (fixes #87) #88

dannyfritz · 2016-09-15T19:16:36Z

Issue (#87)

This pull request is about getting the public, protected, and private keywords into the ESLint AST.

The accessibility modifier keywords can appear on ClassProperty and MethodProperty.

eslintbot · 2016-09-15T19:16:38Z

Thanks for the pull request, @dannyfritz! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.
Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

eslintbot · 2016-09-17T05:30:37Z

LGTM

eslintbot · 2016-09-17T05:31:21Z

LGTM

eslintbot · 2016-09-17T05:33:29Z

LGTM

eslintbot · 2016-09-17T05:35:17Z

LGTM

eslintbot · 2016-09-17T05:36:17Z

LGTM

JamesHenry

Thanks a lot for this, Danny! This is really coming together

JamesHenry · 2016-09-17T15:01:37Z

lib/ast-converter.js

+         * Gets a TSNode's accessibility level
+         * @returns {string} accessibility "public", "protected", or "private"
+         */
+        function getTSNodeAccessibility() {


It's a minor point, but I would usually prefer that TSNode be passed into the helper function so that it is a bit more portable.

I'm sure we'll be refactoring some of this code once the project is a bit more mature and things like this will make that process easier.

JamesHenry · 2016-09-17T15:08:57Z

tests/fixtures/typescript/basics/class-with-accessibility-modifiers.src.ts

+  protected setBar (bar : string) {
+    this.bar = bar;
+  }
+}


Please add at least one example of methods and properties which have multiple TS modifiers on them:

e.g.

class X { public static yo: string = 'Init'; protected get bye() { return 'bye'; } }

dannyfritz · 2016-09-17T16:57:00Z

lib/ast-converter.js

@@ -420,6 +420,8 @@ module.exports = function(ast, extra) {
            return null;
        }

+        var accessibility;


I really don't like this line. I'm going to have it always set the accessibility and just have it default to null instead to clean up code.

dannyfritz · 2016-09-17T17:50:06Z

I'm noticing constructor should default to accessibility: "public" and currently does not.

Also, there is a shorthand in TS for specifying members in the constructor parameters. Are type annotations parsed into the tree yet?

eslintbot · 2016-09-17T17:53:21Z

LGTM

dannyfritz

constructor should have an accessibility of "public".

JamesHenry · 2016-09-17T17:56:32Z

I'm noticing constructor should default to accessibility: "public" and currently does not

I'm not sure what you mean by this. Can you please point me to an example?

Everything in a TypeScript class is public by default (just like a standard ES2015 class), we will only worry about adding accessibility: "public" in the AST for those nodes which have a PublicKeyword as a modifier.

JamesHenry · 2016-09-17T17:58:22Z

Also, there is a shorthand in TS for specifying members in the constructor parameters. Are type annotations parsed into the tree yet?

Yes, we will need to handle this behaviour in a separate PR as it will require some discussion. Thanks for flagging though!

dannyfritz · 2016-09-17T18:00:42Z

Alright, if we're only concerned with explicit accessibility keywords then this should be good.

JamesHenry · 2016-09-17T18:03:39Z

RE: The constructor:

Oh, I missed this comment from you:

I really don't like this line. I'm going to have it always set the accessibility and just have it default to null instead to clean up code.

...and I can see you have already updated all the relevant existing ASTs.

@nzakas What do you think about this change?

If we do go with Danny's latest changes which adds "accessibility" to everything, then constructor should probably have it too

dannyfritz · 2016-09-17T18:04:57Z

@JamesHenry #87 (comment)

JamesHenry · 2016-09-17T18:06:34Z

Ok great, sorry I missed that discussion earlier in the week as I was travelling

JamesHenry · 2016-09-17T18:08:31Z

So, to summarise, I still thinks it makes sense to keep the accessibility value to be null, unless the modifiers are explicitly used. They will only be relevant to specific plugin rules anyway

JamesHenry · 2016-09-17T18:19:57Z

This LGTM now!

I'll leave it to @nzakas to merge to double check he is happy with "accessibility" over "visibility".

We usually try and match existing ESTree or Flow constructs wherever possible, but these modifiers do not exist on classes for them yet. I therefore think it makes sense to go with TypeScript's term of "accessibility", as in this PR.

Thanks again, @dannyfritz!

dannyfritz · 2016-09-18T16:34:42Z

I'm going to add accessibility to the constructor too. null still if nothing is specified. Then i think this PR is ready.

nzakas · 2016-09-19T17:47:31Z

So, to summarise, I still thinks it makes sense to keep the accessibility value to be null, unless the modifiers are explicitly used.

Totally agree.

I'll leave it to @nzakas to merge to double check he is happy with "accessibility" over "visibility".

Eh, I don't have the energy to argue this point, so it's fine. :)

@dannyfritz it looks like you're still planning on making changes? Can you let us know when you do?

eslintbot · 2016-09-21T01:36:14Z

LGTM

dannyfritz · 2016-09-21T01:37:24Z

Alright. This PR is ready for review.

JamesHenry · 2016-09-21T08:10:01Z

Thanks, @dannyfritz! This still LGTM.

jquerybot added the CLA: Valid label Sep 15, 2016

dannyfritz force-pushed the visibility-modifiers branch from a756c19 to c152007 Compare September 17, 2016 05:30

dannyfritz force-pushed the visibility-modifiers branch from c152007 to 5908e5e Compare September 17, 2016 05:31

dannyfritz changed the title ~~🚧 WIP: Visibility Modifiers (#87)~~ New: Accessibility Modifiers (#87) Sep 17, 2016

dannyfritz changed the title ~~New: Accessibility Modifiers (#87)~~ New: Accessibility Modifiers Sep 17, 2016

dannyfritz force-pushed the visibility-modifiers branch from 5908e5e to fb220e3 Compare September 17, 2016 05:33

dannyfritz force-pushed the visibility-modifiers branch from fb220e3 to 07d279d Compare September 17, 2016 05:35

dannyfritz force-pushed the visibility-modifiers branch from 07d279d to 53b7cbe Compare September 17, 2016 05:36

dannyfritz changed the title ~~New: Accessibility Modifiers~~ New: Accessibility Modifiers (fixes #87) Sep 17, 2016

JamesHenry suggested changes Sep 17, 2016

View reviewed changes

dannyfritz commented Sep 17, 2016

View reviewed changes

dannyfritz force-pushed the visibility-modifiers branch from 53b7cbe to 5905cd4 Compare September 17, 2016 17:53

dannyfritz commented Sep 17, 2016

View reviewed changes

JamesHenry approved these changes Sep 17, 2016

View reviewed changes

JamesHenry assigned nzakas Sep 17, 2016

dannyfritz changed the title ~~New: Accessibility Modifiers (fixes #87)~~ WIP: New: Accessibility Modifiers (fixes #87) Sep 18, 2016

dannyfritz force-pushed the visibility-modifiers branch from 5905cd4 to a0ef527 Compare September 21, 2016 01:36

New: Accessibility Modifiers (fixes eslint#87)

a0ef527

dannyfritz changed the title ~~WIP: New: Accessibility Modifiers (fixes #87)~~ New: Accessibility Modifiers (fixes #87) Sep 21, 2016

JamesHenry merged commit 5dae849 into eslint:master Sep 21, 2016

dannyfritz deleted the visibility-modifiers branch October 11, 2016 12:52

Uh oh!

New: Accessibility Modifiers (fixes #87) #88

New: Accessibility Modifiers (fixes #87) #88

Uh oh!

Conversation

dannyfritz commented Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eslintbot commented Sep 15, 2016

Uh oh!

eslintbot commented Sep 17, 2016

Uh oh!

eslintbot commented Sep 17, 2016

Uh oh!

eslintbot commented Sep 17, 2016

Uh oh!

eslintbot commented Sep 17, 2016

Uh oh!

eslintbot commented Sep 17, 2016

Uh oh!

JamesHenry left a comment

Choose a reason for hiding this comment

Uh oh!

JamesHenry Sep 17, 2016

Choose a reason for hiding this comment

Uh oh!

JamesHenry Sep 17, 2016

Choose a reason for hiding this comment

Uh oh!

dannyfritz Sep 17, 2016

Choose a reason for hiding this comment

Uh oh!

dannyfritz commented Sep 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eslintbot commented Sep 17, 2016

Uh oh!

dannyfritz left a comment

Choose a reason for hiding this comment

Uh oh!

JamesHenry commented Sep 17, 2016

Uh oh!

JamesHenry commented Sep 17, 2016

Uh oh!

dannyfritz commented Sep 17, 2016

Uh oh!

JamesHenry commented Sep 17, 2016

Uh oh!

dannyfritz commented Sep 17, 2016

Uh oh!

JamesHenry commented Sep 17, 2016

Uh oh!

JamesHenry commented Sep 17, 2016

Uh oh!

JamesHenry commented Sep 17, 2016

Uh oh!

dannyfritz commented Sep 18, 2016

Uh oh!

nzakas commented Sep 19, 2016

Uh oh!

eslintbot commented Sep 21, 2016

Uh oh!

dannyfritz commented Sep 21, 2016

Uh oh!

JamesHenry commented Sep 21, 2016

Uh oh!

Uh oh!

dannyfritz commented Sep 15, 2016 •

edited

Loading

dannyfritz commented Sep 17, 2016 •

edited

Loading