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 private class features info to no-underscore-dangle #17386

Merged

Conversation

matwilko
Copy link
Contributor

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 autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

no-underscore-dangle docs currently state that 'JavaScript doesn’t have truly private members' and go into detail around the history of underscored identifiers.

Since ES2022/ES13 brought in truly private members, I've slimmed down the historical context and added info about private members and a recommendation to use them instead of underscored identifiers.

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

I removed the historical context because it seems less relevant now that actual private fields can be recommended, but I can restore it if you think it still provides useful context.

As a follow-up, would a proposal for a new rule that highlights leading-underscore members and suggests they be refactored as private members be reasonable? Or should that perhaps be rolled into this rule to prevent any overlap?

@matwilko matwilko requested a review from a team as a code owner July 18, 2023 16:56
@eslint-github-bot eslint-github-bot bot added the documentation Relates to ESLint's documentation label Jul 18, 2023
@netlify
Copy link

netlify bot commented Jul 18, 2023

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit f126324
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/64baabad873bf80008bf1735
😎 Deploy Preview https://deploy-preview-17386--docs-eslint.netlify.app/rules/no-underscore-dangle
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

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 for looking at this. I've left some notes to clean this up a bit but I like the direction.

docs/src/rules/no-underscore-dangle.md Show resolved Hide resolved
docs/src/rules/no-underscore-dangle.md Outdated Show resolved Hide resolved
@matwilko matwilko force-pushed the no-underscore-dangle-private-fields-update branch from 14071a0 to cec536d Compare July 19, 2023 18:06
@matwilko
Copy link
Contributor Author

Put the history back in, but tried to tighten it up a bit.

Put the Private class features notice in an Important box to really highlight it.

@matwilko matwilko force-pushed the no-underscore-dangle-private-fields-update branch 3 times, most recently from 8582d83 to 49e273a Compare July 21, 2023 12:43
@matwilko matwilko force-pushed the no-underscore-dangle-private-fields-update branch from 49e273a to f126324 Compare July 21, 2023 16:00
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 7fc3a2c into eslint:main Jul 24, 2023
22 checks passed
@matwilko matwilko deleted the no-underscore-dangle-private-fields-update branch July 24, 2023 15:35
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jan 21, 2024
@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 Jan 21, 2024
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 contributor pool documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants