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

Fix: Clarify line breaks in object-curly-newline (fixes #14024) #14063

Merged
merged 3 commits into from Mar 24, 2021

Conversation

arminyahya
Copy link
Contributor

Prerequisites checklist

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

  • 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)

I changed some sentences to clarify what inside braces mean for this rule.

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

No.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Feb 2, 2021
@aladdin-add aladdin-add added documentation Relates to ESLint's documentation enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Feb 3, 2021
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion rule Relates to ESLint's core rules and removed enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Feb 3, 2021
@mdjermanovic mdjermanovic linked an issue Feb 10, 2021 that may be closed by this pull request
@@ -1,10 +1,10 @@
# enforce consistent line breaks inside braces (object-curly-newline)
# enforce consistent line breaks after opening and before closing braces (object-curly-newline)
Copy link
Member

Choose a reason for hiding this comment

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

Looks good!

Can we make the same change ("inside braces" -> "after opening and before closing braces") here, in descriptions of string options "never" and "always":

  • "always" requires line breaks after opening and before closing braces
  • "never" disallows line breaks after opening and before closing braces

Also, can we update meta description to be the same as the new title: "enforce consistent line breaks after opening and before closing braces".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now this is similar to https://eslint.org/docs/rules/array-bracket-newline.
I think both of them should be written the same way.
I can change this one and make another PR for array-bracket-newline. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delay. Yes, sounds like a good plan!


A number of style guides require or disallow line breaks inside of object braces and other tokens.

## Rule Details

This rule enforces consistent line breaks inside braces of object literals or destructuring assignments.
This rule enforces consistent line breaks after opening and before closing braces of object literals or destructuring assignments.
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts about adding another sentence below this one, to be even more specific?

Maybe something like: "More precisely, the rule requires or disallows a line break between { and its following token, and between } and its preceding token."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about replacing

This rule enforces consistent line breaks after opening and before closing braces of object literals or destructuring assignments

with:

More precisely, the rule requires or disallows a line break between { and its following token, and between } and its preceding token

?

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to merge these two sentences into one if it would be more understandable.

@arminyahya
Copy link
Contributor Author

arminyahya commented Feb 19, 2021

Pardon me for I haven't noticed your code review, my mailbox might have had a problem in receiving emails.

@mdjermanovic mdjermanovic changed the title Docs: Clarify line breaks in object-curly-newline (refs #14024) Fix: Clarify line breaks in object-curly-newline (fixes #14024) Mar 12, 2021
@mdjermanovic
Copy link
Member

I changed the PR title to tag this as Fix:, since there are changes in rule metadata.


A number of style guides require or disallow line breaks inside of object braces and other tokens.

## Rule Details

This rule enforces consistent line breaks inside braces of object literals or destructuring assignments.
More precisely, the rule requires or disallows a line break between { and its following token, and between } and its preceding token of object literals or destructuring assignments.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
More precisely, the rule requires or disallows a line break between { and its following token, and between } and its preceding token of object literals or destructuring assignments.
This rule requires or disallows a line break between `{` and its following token, and between `}` and its preceding token of object literals or destructuring assignments.

Since this is now the first sentence in this section, it wouldn't be clear what "More precisely" refers to.

Also added backticks around { and }

Copy link
Member

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

@mdjermanovic mdjermanovic merged commit a99eb2d into eslint:master Mar 24, 2021
13 checks passed
@mdjermanovic
Copy link
Member

Thanks for contributing!

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Sep 21, 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 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion 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.

'ImportDeclaration': 'never', is not enforced.
3 participants