Skip to content
This repository was archived by the owner on Jan 19, 2019. It is now read-only.

New: Add rule explicit-member-accessibility #4

Merged
merged 1 commit into from
Oct 3, 2016

Conversation

dannyfritz
Copy link
Contributor

Rule based off of TSLint's member-access rule

@JamesHenry
Copy link
Collaborator

Thanks so much @dannyfritz, it's fantastic to have another contributor already!

@nzakas I do not have the right to create a travis job for this repo, please could you do it in the same way we have for typescript-eslint-parser to save us having to manually run the tests for contributor PRs?

A few thoughts on the PR:

  • Let's make sure our docs are entirely free-standing, and do not require linking to TSLint documentation. This project is not intended to be a replacement/emulation of TSLint, and whilst it is naturally a fantastic resource for us when designing rules, I don't think the docs should be linked. The implementations, capabilities and naming of the rules in the respective projects may well diverge over time, so it's good to keep them separate.
  • Other than removing the link to TSLint, the documentation is currently incomplete
  • You've used the term 'decorator' a couple of times when I believe you meant 'declaration'. We need to be careful with using decorator as it naturally has a fixed meaning in the context of TypeScript

Thanks again, Danny!

@dannyfritz
Copy link
Contributor Author

dannyfritz commented Sep 12, 2016

  • I'll remove the link to TSLint. However, I'd request an issue with checkboxes for each TSLint rule implemented so far. Just like how ESLint had a push to do all the rules JSHint does, I think we should have a push for feature parity with TSLint as that is currently the standard.
  • I'll spend some time today to get the docs more filled out.
  • Yeah, I thought it was a visibility decorator when I started, but as I was munging through ts.SyntaxType, I noticed it was not.

I have a question for someone more knowledgeable: It doesn't seem ts parses the visibility keywords into the AST unless I'm mistaken. I ended up having to grab the sourcecode and do a RegExp test for the keywords on the classProperty and methodDeclaration. I don't really care for this approach. Does anyone know of a better way to find them using the AST? Or really, where can I find documentation for ts.languageService?

@dannyfritz
Copy link
Contributor Author

@nzakas
Copy link
Contributor

nzakas commented Sep 12, 2016

@dannyfritz feel free to open an issue with checkboxes for all the TSLint rules.

wrt to your AST question, if something is missing from the AST, the right place to discuss that is on the typescript-eslint-parser repo to see if we can do something about it.

@nzakas
Copy link
Contributor

nzakas commented Sep 12, 2016

@JamesHenry just setup Travis. I'm not sure why you couldn't, but I'll double-check.

And yeah, let's stick with the ESLint conventions for commit messages because that will let us use the same release tool.

@dannyfritz dannyfritz mentioned this pull request Sep 12, 2016
31 tasks
@dannyfritz
Copy link
Contributor Author

Renamed Visibility Declaration to Visibility Modifier as per the TypeScript docs.


Followed ESLint's contributing guidelines.

dannyfritz added a commit to dannyfritz/eslint-plugin-typescript that referenced this pull request Sep 12, 2016
dannyfritz added a commit to dannyfritz/eslint-plugin-typescript that referenced this pull request Sep 12, 2016
@nzakas
Copy link
Contributor

nzakas commented Sep 13, 2016

Code looks good overall (I'm fixing Travis right now). I wonder if we can come up with a better rule name, though. member-access doesn't seem to indicate what the rule actually does. If it enforces the use of public or private, maybe a better name would be class-member-visibility?

@dannyfritz dannyfritz changed the title ✨ member-access rule WIP ✨ member-access rule Sep 18, 2016
dannyfritz added a commit to dannyfritz/eslint-plugin-typescript that referenced this pull request Sep 25, 2016
@dannyfritz
Copy link
Contributor Author

Renamed to use-accessibility and bumped version of typescript-eslint-parser to get new accessibility features.

@dannyfritz dannyfritz changed the title WIP ✨ member-access rule NEW: Add rule use-accessibility Sep 25, 2016
@dannyfritz dannyfritz changed the title NEW: Add rule use-accessibility New: Add rule use-accessibility Sep 25, 2016
dannyfritz added a commit to dannyfritz/eslint-plugin-typescript that referenced this pull request Sep 25, 2016
@JamesHenry
Copy link
Collaborator

@dannyfritz @nzakas how do you feel about explicit-class-member-accessibility or explicit-member-accessibility as the name of the rule?

My reasoning:

  • Including class-member- / member- in the name, because "accessibility" is too generic a term to not include some kind of context
  • "explicit-" prefix over "use-", because you are not opting into a behaviour, or making use of a feature of accessibility as such (members are public by default, so adding a "public" modifier simply makes this behaviour explicit

@AdamWillden
Copy link

Just to chime in as an outsider excitedly watching the progress going on here (fantastic work all of you!)...

I agree with @JamesHenry on the naming. Not precious about which but the shortness of explicit-member-accessibility probably wins it out of the two, for me.

@nzakas
Copy link
Contributor

nzakas commented Sep 29, 2016

Yeah, I like explicit-member-accessibility as I think it clearly describes what the rule is doing (at first, I wasn't sure if it was also checking for public or private elsewhere).

…y modifiers such as public, private, and protected. (refs bradzacher#4)
@dannyfritz dannyfritz changed the title New: Add rule use-accessibility New: Add rule explicit-member-accessibility Oct 3, 2016
@dannyfritz
Copy link
Contributor Author

Renamed to explicit-member-accessibility and should be ready.

@nzakas nzakas merged commit a879313 into bradzacher:master Oct 3, 2016
@nzakas
Copy link
Contributor

nzakas commented Oct 3, 2016

Cool! I'll publish a new release momentarily.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants