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: Make rule descriptions more consistent #5778

Merged
merged 1 commit into from Apr 11, 2016

Conversation

scriptdaemon
Copy link
Contributor

For each rule doc, I made the title consistent with the description in the README.md file. Since I also made some grammar changes for better consistency between rules, I want to make sure those changes are acceptable as well.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @alberto, @kaicataldo and @onurtemizkan to be potential reviewers

@eslintbot
Copy link

LGTM

@kaicataldo
Copy link
Member

Is there an issue open for this? I can make more specific comments later (on my commute to work right now), but I feel like some of these changes make what the respective rule enforces less clear.

I'm all for consistency as long as it improves clarity though :)

@kaicataldo
Copy link
Member

Sorry, let me actually rephrase my question: What's the motivation behind this change? I've personally never felt like inconsistencies in the rule descriptions were problem.

From my quick scan it looked to me like some of changes sacrifice clarity and detail in the descriptions for more consistent word choice, which is not a good trade off in my opinion.

Never mind, I understand now. I totally agree that having consistency between the readme and rule docs would be great.

* [valid-jsdoc](valid-jsdoc.md): ensure JSDoc comments are valid
* [valid-typeof](valid-typeof.md): ensure results of typeof are compared against a valid string (recommended)
* [no-unexpected-multiline](no-unexpected-multiline.md): disallow unexpected multiline expressions (recommended)
* [no-unreachable](no-unreachable.md): disallow unreachable code (recommended)
Copy link
Member

Choose a reason for hiding this comment

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

I personally prefer the previous version - this seems less helpful to me

@kaicataldo
Copy link
Member

@scriptdaemon Okay, now I'm at a computer! I should have said this earlier - thanks for the PR :)

Generally, we like for there to be an accepted issue so we can hammer out the details before any work is done and ensure it's a change we want to make (as well as making sure people aren't putting time into something that may or may not be used).

I love the idea of your PR - we should definitely have parity between the rule descriptions in the readme and the rule docs themselves. You also bring up some interesting inconsistencies where I'm not sure what the right way forward is (the "one true comma style" one is a good example). Let's see what the rest of the team has to say.

@nzakas
Copy link
Member

nzakas commented Apr 4, 2016

@kaicataldo doc changes don't require issues so long as the purpose is obvious.

I think we have to be careful not to undo @pedrottimark's work on this, so let's get his input before proceeding.

@kaicataldo
Copy link
Member

@nzakas Ah okay, thanks for clarifying 👍

@pedrottimark
Copy link
Member

Super effort to make rule descriptions more clear and consistent!

Here are suggestions how to move forward:

@pedrottimark
Copy link
Member

Concerning the verbs, in a first look at the patterns for describing options in rule docs, the contrasting verbs seem to be enforces and disallows to describe positive versus negative constraints.

@ilyavolodin
Copy link
Member

I would replace encourage with enforce or require. Same with ensure. Remove specify and treat. So you left with disallow, enforce/require and restrict.
I'm perfectly fine with switching README.md to the table as displayed in your examples. I think it will be a bit easier to consume from the user perspective.

@scriptdaemon
Copy link
Contributor Author

@kaicataldo In the PR for max-statements-per-line I was asked to keep the title of the rule doc and the description of the rule in the README.md consistent, so I wanted to apply that consistency across the board. I'm definitely glad you have concerns, as wording can be very subjective, and I agree with the ones you have so far.

@pedrottimark Thank you. I knew you were working on doc updates as well so I was hoping to have your input (I suppose I could have just pinged you myself =P). For each of your bullet points:

  • Makes sense to me. I'll update this commit when time permits so that it only involves updating the language of the descriptions in the README.md. Also, I wouldn't mind helping out with Docs: WIP Guidelines for rule pages #5446 at all.
  • Given how much I was able to reduce in the PR as it stands right now, I agree with @ilyavolodin. I think I tried too hard to reduce in some rules to the point where it may have made the wording a little awkward, so opening it back up to allow require and restrict would be better.
  • 👍 for tables.

@pedrottimark
Copy link
Member

@scriptdaemon What about this pattern to replace some “enforce or disallow” pairs with enforce?

  • array-bracket-spacing: enforce spacing style inside array brackets (fixable)
  • block-spacing: enforce spacing style inside single-line blocks (fixable)
  • brace-style: enforce brace style for blocks
  • camelcase: enforce camel case style for names
  • comma-spacing: enforce spacing style before and after commas (fixable)

As long as X style is not too vague.

@scriptdaemon
Copy link
Contributor Author

@pedrottimark I like that. That helps keep some of the rules' descriptions more succinct,

@scriptdaemon
Copy link
Contributor Author

@pedrottimark Now that I've gotten down to the stylistic rules in the README, I took your idea and made some adjustments that I think read a bit easier (I'm not against changing them back if you disagree). Let me know what you think.

  • array-bracket-spacing: enforce consistent spacing inside array brackets (fixable)
  • block-spacing: enforce consistent spacing inside single-line blocks (fixable)
  • brace-style: enforce consistent brace style for blocks
  • camelcase: enforce camelcase naming convention
  • comma-spacing: enforce consistent spacing before and after commas (fixable)

EDIT: I amended the commit to include my current changes. Please look over my wording to see if you agree or think anything should be changed.

@scriptdaemon scriptdaemon changed the title Docs: Use consistent titles Docs: Make rule descriptions more consistent Apr 6, 2016
* [brace-style](brace-style.md): enforce consistent curly brace style for blocks
* [camelcase](camelcase.md): enforce camelcase naming convention
* [comma-spacing](comma-spacing.md): enforce consistent spacing before and after commas (fixable)
* [comma-style](comma-style.md): enforce consistent comma style
* [computed-property-spacing](computed-property-spacing.md): require or disallow padding inside computed properties (fixable)
Copy link
Member

Choose a reason for hiding this comment

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

enforce consistent spacing inside computed property brackets (fixable)

* [operator-assignment](operator-assignment.md): require assignment operator shorthand where possible or prohibit it entirely
* [operator-linebreak](operator-linebreak.md): enforce operators to be placed before or after line breaks
* [padded-blocks](padded-blocks.md): enforce padding within blocks
* [one-var](one-var.md): enforce variables to be declared at the same time or separately per function
Copy link
Member

Choose a reason for hiding this comment

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

declared either together or separately

You might think of an even clearer word to communicate the contrast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one actually took me the longest to figure out a wording that wasn't completely awkward. I like your suggestion better.

@pedrottimark
Copy link
Member

@scriptdaemon 👍 👍 to your improved wording. You are brave to work on 227 rule descriptions, sir.

Commented on Stylistic Issues. If it is helpful, can look at other sections.

@scriptdaemon
Copy link
Contributor Author

@pedrottimark Thank you for the feedback. I'll go through your changes, then apply whatever changes necessary to the rules in the other sections to remain consistent.

I raised a few questions in your PR (e.g. whether we should use plural vs singular), and depending the direction you think we should take I'll factor those changes into the rule descriptions as well.

@scriptdaemon
Copy link
Contributor Author

@pedrottimark All changes as discussed so far have been implemented.

* [no-extra-boolean-cast](no-extra-boolean-cast.md): disallow double-negation boolean casts in a boolean context (recommended)
* [no-empty-character-class](no-empty-character-class.md): disallow empty character classes in regular expressions (recommended)
* [no-ex-assign](no-ex-assign.md): disallow reassigning exceptions in `catch` clauses (recommended)
* [no-extra-boolean-cast](no-extra-boolean-cast.md): disallow double-negation boolean casts (recommended)
Copy link
Member

Choose a reason for hiding this comment

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

disallow unnecessary boolean casts

The original description had 2 issues:

  • “boolean cast” refers to either !! or Boolean()
  • “in a boolean context” was trying to communicate something similar to “unnecessary” in no-extra-parens

@pedrottimark
Copy link
Member

Added 4 line comments where there was an inconsistency between original description and current rule options, plus 1 nitpick for stray period.

Other than those, LGTM

@scriptdaemon You reached beyond “low-hanging fruit” here. I had a gut instinct when you wrote that.

@scriptdaemon
Copy link
Contributor Author

@pedrottimark Done.

The progress on this is looking good. Now just waiting on other feedback.

Once this is merged, when do we want to update the doc titles for each rule?

@pedrottimark
Copy link
Member

@scriptdaemon Will probably merge on Monday because descriptions are on critical path for #5417

Let’s update the main headings in docs as part of the task to develop and implement guidelines for example sentences and option descriptions:

@scriptdaemon
Copy link
Contributor Author

@pedrottimark That all sounds good to me.

Do you have a collected list of the ones you've already done? If you haven't done the rule under the "strict" category yet, I can quickly do that tomorrow to see if I have in mind the correct way you want these done.

@pedrottimark
Copy link
Member

@scriptdaemon Great minds think alike. I started with Variables because it was a small section, but then worked from the beginning through the first 10 in Stylistic Issues. So there are no gaps.

Because I included strict in #5596 the next best thing is try global-strict under Removed

By the way, I meant 10 to 15 as an upper bound for the sake of the reviewers and to reduce chance of merge conflicts. Smaller batches are fine.

@scriptdaemon
Copy link
Contributor Author

@pedrottimark Ah, gotcha.

The reason I mentioned the one under strict was that it quick since that category only contains one rule, not because I only intended to start with one rule. I can do the next 10-15 from where you left off no problem since you already did it.

@pedrottimark pedrottimark merged commit 62b2526 into eslint:master Apr 11, 2016
@scriptdaemon scriptdaemon deleted the docs-consistent-titles branch April 11, 2016 23:11
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
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

8 participants