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 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
Copy link

jsf-clabot commented Jan 20, 2018

CLA assistant check
All committers have signed the CLA.

@platinumazure
Copy link
Member

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
Copy link
Member

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

@dandv
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
Copy link
Member

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
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 platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@platinumazure
Copy link
Member

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 kaicataldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for contributing!

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

4 participants