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

Update docs to make fix param more clear in NodeJS API #6367

Merged
merged 1 commit into from Jun 13, 2016

Conversation

NickHeiner
Copy link
Contributor

See #6366.

@eslintbot
Copy link

Thanks for the pull request, @NickHeiner! 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.)

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @nzakas, @gyandeeps and @ilyavolodin to be potential reviewers

@jquerybot
Copy link

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.


If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

@eslintbot
Copy link

LGTM

@platinumazure
Copy link
Member

I'd love to see a quick sentence in executeOnFiles() as well, just in case that's where someone is reading. What do you think?

@eslintbot
Copy link

LGTM

@NickHeiner
Copy link
Contributor Author

Yep, that makes sense to me. I've signed the CLA, but it is not yet showing up in the PR checks. Does that take some time to propagate?

@platinumazure
Copy link
Member

platinumazure commented Jun 10, 2016

Regarding jQuery CLA, I know that it's very finicky about your name and e-mail address (which you specified on signing the CLA). The e-mail address has to exactly match the git config user.email (or git config --core user.email) value you have on your local Git. Can you confirm that they're the same?

EDIT: I just checked the check status page, and it indicates that the commit e-mail starts with NickHeiner@ and the CLA signature e-mail starts with nickheiner@. Try correcting the CLA signature e-mail to NickHeiner@users.noreply.github.com and see if that fixes everything? Actually, seems jQuery CLA just doesn't accept e-mail addresses with that domain name, in case they need to contact you or whatnot. My apologies, but you're going to need to configure Git, GitHub, and the CLA with a different e-mail.

@mysticatea
Copy link
Member

If my memory is correct, jQuery CLA does not accept noreply.github.com mail address because the mail address does not have the ability to receive mails.

@platinumazure
Copy link
Member

Doc changes LGTM, but I would be grateful if @nzakas or @pedrottimark could take a look.

@platinumazure
Copy link
Member

@NickHeiner Regarding jQuery CLA: Actually, seems jQuery CLA just doesn't accept e-mail addresses with that domain name, in case they need to contact you or whatnot. My apologies, but you're going to need to configure Git, GitHub, and the CLA with a different e-mail. Sorry for missing that earlier.

@eslintbot
Copy link

LGTM

Updating the NodeJS API docs to avoid surprising developers using the `fix` param.
@eslintbot
Copy link

LGTM

@NickHeiner
Copy link
Contributor Author

Ok, I've fixed the email / CLA issue. Thanks for your help on that.

@ilyavolodin
Copy link
Member

Thanks @NickHeiner LGTM.

@ilyavolodin ilyavolodin merged commit 7f57467 into eslint:master Jun 13, 2016
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants