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: Add valid example that shows vars in a block scope #14230

Merged
merged 2 commits into from Mar 25, 2021

Conversation

edg2s
Copy link
Contributor

@edg2s edg2s commented Mar 18, 2021

Prerequisites checklist

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

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

All the current valid examples show hoisting the var to the function scope. This example makes is clear that this rule allows you to declare vars inside a block scope, just not use them out of that scope.

Is there anything you'd like reviewers to focus on?

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Mar 18, 2021
@eslint-github-bot
Copy link

Hi @edg2s!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag must be one of the following:

    The Tag is one of the following:

    • Fix - for a bug fix.
    • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
    • New - implements a new feature.
    • Breaking - for a backwards-incompatible enhancement or feature.
    • Docs - changes to documentation only.
    • Build - changes to build process only.
    • Upgrade - for a dependency upgrade.
    • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

    You can use the labels of the issue you are working on to determine the best tag.

  • There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

@edg2s edg2s changed the title Add valid example that shows vars in a block scope Docs: Add valid example that shows vars in a block scope Mar 18, 2021
@eslint-github-bot
Copy link

Hi @edg2s!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag must be one of the following:

    The Tag is one of the following:

    • Fix - for a bug fix.
    • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
    • New - implements a new feature.
    • Breaking - for a backwards-incompatible enhancement or feature.
    • Docs - changes to documentation only.
    • Build - changes to build process only.
    • Upgrade - for a dependency upgrade.
    • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

    You can use the labels of the issue you are working on to determine the best tag.

  • There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

All the current valid examples show hoisting the var to the function scope. This example makes is clear that this rule allows you to declare vars inside a block scope, just not use them out of that scope.
@nzakas nzakas added documentation Relates to ESLint's documentation rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Mar 19, 2021
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Thanks. Can you add a corresponding example in the “invalid” section, too? Note that all of the current examples in the “valid” section are rewritten versions of the code in the “invalid” section, and that is by design.

@edg2s
Copy link
Contributor Author

edg2s commented Mar 20, 2021

I did notice that, but I don't think there is a functionally equivalent invalid example for this function. I could move the console.log down but that would be different functionality.

@nzakas
Copy link
Member

nzakas commented Mar 22, 2021

You may want to consider a different example, then, that can show the same thing.

@edg2s
Copy link
Contributor Author

edg2s commented Mar 23, 2021

You may want to consider a different example, then, that can show the same thing.

Any invalid case would have a variable being used outside the scope in which it was defined. In order to fix that without changing the functionality you would have to move the variable declaration further up, but the whole point of this PR is to add an example of a var in a block scope.

@edg2s
Copy link
Contributor Author

edg2s commented Mar 23, 2021

Note that all of the current examples in the “valid” section are rewritten versions of the code in the “invalid” section, and that is by design.

And there are many other places where the examples don't (or can't) match up perfectly, e.g. https://eslint.org/docs/rules/no-compare-neg-zero, https://eslint.org/docs/rules/valid-typeof, https://eslint.org/docs/rules/dot-location, ...

@nzakas
Copy link
Member

nzakas commented Mar 24, 2021

The goal isn’t necessarily to show a bad example and then the fixed version, it’s to show an example of what the rule will flag and then a similar example that shows what the rule won’t flag.

Despite other rules not always following this pattern, this rule’s documentation does, so I’d prefer to stick with it.

nzakas
nzakas approved these changes Mar 25, 2021
Copy link
Member

@nzakas nzakas 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!

@nzakas nzakas merged commit 909c727 into eslint:master Mar 25, 2021
13 checks passed
@edg2s edg2s deleted the patch-1 branch Mar 25, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Sep 22, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Sep 22, 2021
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 documentation Relates to ESLint's documentation rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants