Skip to content

Add support for inferring private based on the name #441

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

Merged
merged 1 commit into from
Jun 1, 2016

Conversation

arv
Copy link
Contributor

@arv arv commented Jun 1, 2016

This adds a command line flag called --infer-private which is a
string (defaults to ^_) which is used as a regexp for inferring
if a name is private or not.

For example:

/** C */
class C {
  /** I'm public */
  m() {}
  /** I'm private */
  _p() {}
}

Fixes #436

@arv
Copy link
Contributor Author

arv commented Jun 1, 2016

I wanted to add an end to end test since this does not test that the command line flag gets passed along all the way [*] but I wasn't sure how to best do that. Any suggestions?

[*] it does from manual testing

@tmcw
Copy link
Member

tmcw commented Jun 1, 2016

Great! The test/bin tests are where we do end-to-end testing. They don't have the same automatic fixture generation as test/test, but you could run assertions off of the output.

@arv arv force-pushed the infer-private branch from 3053f3e to cc3012a Compare June 1, 2016 17:37
@arv
Copy link
Contributor Author

arv commented Jun 1, 2016

I added a test to test/bin.js.

This is ready for review.

@tmcw
Copy link
Member

tmcw commented Jun 1, 2016

On a code level, it looks good to me, but I can't see how one would turn off inference entirely; you can replace the regex with a different one, but in order to "never infer private methods", I am inferring that you would have to write a regular expression that does not match anything, and for me that's a hard thing to guess at.

@arv
Copy link
Contributor Author

arv commented Jun 1, 2016

That is a good point. Two alternatives.

  1. Leave infer private off by default
  2. Treat an empty string as off.

I'm leaning towards 1.

@arv
Copy link
Contributor Author

arv commented Jun 1, 2016

The option is now off by default. Let me know if you want me to squash the commits?

@tmcw
Copy link
Member

tmcw commented Jun 1, 2016

👍 Excellent, yep, squash and I'll merge!

@arv arv force-pushed the infer-private branch 3 times, most recently from 1113330 to 98723e9 Compare June 1, 2016 21:36
This adds a command line flag called `--infer-private` which is a
string (defaults to `^_`) which is used as a regexp for inferring
if a name is private or not.

For example:

```js
/** C */
class C {
  /** I'm public */
  m() {}
  /** I'm private */
  _p() {}
}
```

Fixes documentationjs#436
@arv arv force-pushed the infer-private branch from 98723e9 to 629ea6f Compare June 1, 2016 21:40
@arv
Copy link
Contributor Author

arv commented Jun 1, 2016

Squashed and some minor cleanup

@tmcw
Copy link
Member

tmcw commented Jun 1, 2016

👍 thanks, merging!

@tmcw tmcw merged commit bf0f100 into documentationjs:master Jun 1, 2016
@arv arv deleted the infer-private branch June 2, 2016 23:48
rhendric pushed a commit to rhendric/documentation that referenced this pull request Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants