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

Docs: Mention the `globals` key in the no-undef docs #9867

Merged
merged 3 commits into from Jan 22, 2018

Conversation

@dandv
Copy link
Contributor

dandv commented Jan 20, 2018

What is the purpose of this pull request? (put an "X" next to item)

[x] Documentation update

What changes did you make? (Give an overview)
Added a sentence to no-undef mentioning the globals key.

@jsf-clabot

This comment has been minimized.

Copy link

jsf-clabot commented Jan 20, 2018

CLA assistant check
All committers have signed the CLA.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Jan 20, 2018

Hi @dandv, thanks for the PR. Before we can merge this (either as it is now or after possible changes), we'll need you to sign our Contributor License Agreement as noted here. Thanks!

I'm not 100% sure about adding a change to that section. The very first section in our rule docs is usually meant to be a general description about what the rule is trying to solve, and we don't normally put specific implementation or configuration details there. So my suggestion would be to take a look at the rest of the docs and see if the globals configuration is mentioned in good places (e.g., inline in the configuration/options sections, and/or in the See Also section).

If you believe that global variables still need to be mentioned in the first section, please keep it general, high-level, and free of configuration information (for example, mention that the use of variables not defined in function or module scope can lead to problems, unless the variables are defined in global scope elsewhere; and then in later sections, make sure the docs cover exactly how to declare those globals so that no-undef will not flag them).

Thanks again for contributing, we really appreciate it!

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Jan 20, 2018

Also, please see our PR guidelines for info on how your commit message and/or PR title should be formatted. Thanks! 😄

@dandv

This comment has been minimized.

Copy link
Contributor Author

dandv commented Jan 20, 2018

Thanks @platinumazure. The reason I stopped by to submit this PR is that it was the second time I found myself using a global exported by a script imported from the HTML file, and I didn't see any link from the no-undef documentation to how to specify globals, yet I remembered JSLint provided that. To save others the time, I do believe it would help to mention, briefly, the globals key in the first section, which is what a visitor will first see when they land here from a web search for no-undef. I think that 'no-undef' is pretty self-explanatory, and the primary reason a developer would refer to the documentation for it is to see how to specify such globals.

PS: I submitted the PR through GitHub, so I can't do a push -f. Please squash & merge if the commit looks OK now.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Jan 20, 2018

Hi @dandv, I absolutely understand and agree with the rationale for including information about globals in the no-undef rules. I'm just saying it's in the wrong section. Notice that the rest of the section only talks about ReferenceErrors and misspelled variable names- it doesn't talk about how the rule works at all.

Please feel free to add something like "except for global variables" or "without warning for known global variables" in the first section. In the second section (Rule Details), please absolutely put anything that you think needs to be added about globals configuration, since that is more about how the rule is actually used.

(Personally, I don't see a lot of value in having the high-level background section sometimes- people come to the docs wanting to know how the rules work. So I can understand why you might find my request a little hard to swallow. But we do follow a set of documentation guidelines right now, and I don't believe your proposed change as written right now fits those guidelines. I apologize if this is becoming a frustrating exercise as a result. I do want our no-undef documentation to be clear and I do want it to reference the globals and make users' lives easier. So I appreciate your patience as we work get this in.)

@dandv

This comment has been minimized.

Copy link
Contributor Author

dandv commented Jan 21, 2018

No worries, thank you for the much kinder guidance than what I've seen from other projects. It does make sense to move my sentence to the "Rule Details" section. Please let me know how it fits now, and please feel free to make changes. I just wanted that information to be somewhere in the no-undef page.

Copy link
Member

platinumazure left a comment

LGTM, thanks!

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Jan 21, 2018

Oops, one small change request. Could you please edit the PR title to begin with "Docs: ", to match our commit message guidelines? Thanks!

@dandv dandv changed the title Mention the `globals` key Docs: Mention the `globals` key Jan 21, 2018
Copy link
Member

kaicataldo left a comment

LGTM. Thanks for contributing!

@kaicataldo kaicataldo changed the title Docs: Mention the `globals` key Docs: Mention the `globals` key in the no-undef docs Jan 22, 2018
@kaicataldo kaicataldo merged commit 9cbb487 into eslint:master Jan 22, 2018
5 checks passed
5 checks passed
commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@renovate renovate bot mentioned this pull request Feb 14, 2018
@renovate renovate bot mentioned this pull request Mar 23, 2018
@eslint eslint bot locked and limited conversation to collaborators Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.