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

Show if rule is enabled by eslint:recommended on rule page #5774

Closed
rik opened this issue Apr 2, 2016 · 40 comments · Fixed by singapore/lint-condo#244 or renovatebot/renovate#123 · May be fixed by iamhunter/teammates#4
Closed

Show if rule is enabled by eslint:recommended on rule page #5774

rik opened this issue Apr 2, 2016 · 40 comments · Fixed by singapore/lint-condo#244 or renovatebot/renovate#123 · May be fixed by iamhunter/teammates#4
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 enhancement This change enhances an existing feature of ESLint help wanted The team would welcome a contribution from the community for this issue rule Relates to ESLint's core rules

Comments

@rik
Copy link

rik commented Apr 2, 2016

Website change so I skipped the eslint version questions.

What did you do? Please include the actual source code causing the issue.
Land on a rule page from a search engine.

What did you expect to happen?
See if the rule is enabled in "eslint:recommended".

What actually happened? Please include the actual, raw output from ESLint.
No indication of the rule being enabled.

The indication is already on http://eslint.org/docs/rules/ so it would be nice to also have it on rule pages.

It would also be useful to see if eslint --fix can fix this. I can open another issue for this though.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Apr 2, 2016
@alberto
Copy link
Member

alberto commented Apr 2, 2016

There is already a notice for fixable rules

@alberto alberto added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules documentation Relates to ESLint's documentation 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 Apr 2, 2016
@pedrottimark
Copy link
Member

Stay tuned to critique some pictures of a possible design on Monday.

@pedrottimark
Copy link
Member

pedrottimark commented Apr 11, 2016

Expanding the scope a bit, here are sentences which could appear after the main heading in rule docs.

The following pictures are 768px width simulated iPad in portrait orientation. What do you think?

recommended
5774 768 recommended comma-dangle

recommended and fixable
5774 768 recommended fixable no-extra-semi

fixable
5774 768 fixable no-multi-spaces

removed
5774 768 removed generator-star

removed and (was) fixable: I am guessing the audience to know that a removed rule was fixable is people using an older version of ESLint, especially soon after it was removed?
5774 768 removed fixable space-after-keywords

@pedrottimark
Copy link
Member

The following pictures are 600px width simulated Nexus 7 in portrait orientation. What do you think?

  • Right now, a media query omits the icons because the margin is too narrow, but we could display them inline instead.
  • If we go in that direction, it probably implies moving the thumb-down and thumbs-up from the code well to the preceding example sentence. See no-extra-semi in this and the next comment.

recommended
5774 600 recommended comma-dangle

recommended and fixable
5774 600 recommended fixable no-extra_semi

fixable
5774 600 fixable no-multi-spaces

removed
5774 600 removed generator-star

removed and (was) fixable
5774 600 removed fixable space-after-keywords

@pedrottimark
Copy link
Member

Here is possible change to position of icons for incorrect and correct code at 768px width. Only a small minority of rule docs will have separate adjacent code fences (for example, vars-on-top).

5774 768 incorrect correct no-extra-semi

@rik
Copy link
Author

rik commented Apr 15, 2016

Sorry for the delay answering. I like this a lot.

In light of #5858, let me propose an alternate sentence:

✔︎ This rule can be enabled by adding one of the following properties in a [configuration file](link):
   - `"extends": "eslint:recommended"`

@pedrottimark
Copy link
Member

@rik Your comment shines some light on a vague thought that was lurking in the back of my mind. For many people, ESLint plugins or JSCS presets seem more relevant than eslint:recommended but those are maintained by third parties, true?

In contrast, a change to which rules are recommended by ESLint is considered breaking, can only happen at major versions. therefore that information is realistic to include in the rules index page (and therefore in rule doc pages).

How to balance displaying information that is most relevant to a particular person with information that is “stable” enough for eslint.org is stretching my mind at this moment.

Could you look at #5417 and tell me what you think about this way forward:

  • we add the simple limited sentence to rule docs on eslint.org
  • because the community can use meta properties in rules to display dynamic relevant lists or tables of rules enabled in plugins or presents (and which can link to rule docs the Web site)

@pedrottimark
Copy link
Member

@nzakas Am glad to receive your guidance about strategic goal and tactical plans to move forward.

Proposed goals are if you click a link from search results or error results to a rule page:

  • Be able to see at a glance whether or not a rule is fixable, removed, or eslint:recommended (page currently displays fixable and removed).
  • Be able to ignore the info if you already know or don’t care (support intentional banner blindness).

See pictures in #5774 (comment)

Proposed plan is 3 stages. Suggest stage 1 in one update version of ESLint, 2 in the next version. 3 might or might not be in the same version as 2 (see below).

  1. 18 removed rules are the warm-up exercise:
    • Make the sentences consistent in wording (they are already similar).
    • Display the X glyphicon for visual similarity with wrench icon at left of sentence in fixable rules.
    • Update the description in the main heading and move the rule identifier to the beginning (for similarity with the rules index). Depends on a change in Makefile.js to match identifier + “: “ at the beginning in addition to identifier enclosed in parentheses at the end of the heading.
    • Reorganize the Removed section in the rules index page as a two-column table of Removed and Replaced by. Just the rule identifiers: no rule description, nor wrench icon for fixable rules.
  2. 212 active rules (of which 43 are recommended) in about 5 batches:
    • Update the description in the main heading and move the rule identifier to the beginning (for similarity with the rules index).
    • Add the recommended sentence which has the check mark glyphicon for visual similarity with the rules index page and wrench icon at left of sentence in fixable rules.
    • Update or delete the few remaining links from active rules to removed rules.
    • When there are no open pull requests for new rules whose docs have headings in the original format, remove from Makefile.js the regexp to match identifier enclosed in parentheses at the end.
  3. rules index page:
    • Reorganize the active sections as three-column tables of recommended icon, fixable icon, rule identifier link + “: “ + rule description. [Am glad for advice from @ilyavolodin whether to do an initial reorganization, and then automate the index generation later; or wait until it is time to automate it]

@nzakas
Copy link
Member

nzakas commented May 4, 2016

@pedrottimark all sounds great to me!

What I'd like to get to eventually is to have the rule documentation title, fixable sentence, recommended sentence, and removed sentence automatically added at build time (so people don't have to manually copy those everywhere). We can get the data from the meta info for each rule and just insert it when we do a build.

That's not really a blocker for your suggestions, just wanted to give you a peak into what I'm thinking.

In any event, I trust your instincts the plan looks good to me.

@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels May 4, 2016
@pedrottimark
Copy link
Member

@nzakas Agree with the goal to automate. I will start with manual changes for stage 1 and brain storm with @ilyavolodin and @vitorbal about stages 2 and 3.

@ilyavolodin
Copy link
Member

@pedrottimark Metadata updates have been completed, I think we should automate index page first and then restructure the presentation (or do both at the same time).

@pedrottimark
Copy link
Member

@ilyavolodin Yes good, let’s automate the rules index first as lists, and then restructure as tables later as simple change.

Exception to that is I will submit a pull request tonight to restructure the Removed section which we cannot build from rule meta data as pictured in eslint/archive-website#234 (comment)

@ilyavolodin
Copy link
Member

👍 Totally forgot about removed rules. Thanks for the reminder. But that will just be added to a template that we use to generate index.

@pedrottimark
Copy link
Member

Yes, just submitted #6091 so you will have stable content for the template.

@pedrottimark
Copy link
Member

Comment to remind us of another exception for removed rules to our automation goal: probably not able to prepend to their doc the main heading, removed paragraph, nor fixable paragraph.

Just as well that we plan not to remove any more for a year or more.

@nzakas
Copy link
Member

nzakas commented May 5, 2016

We can probably do that by just leaving the removed rule docs. In general, I think the various paragraphs should be in the template and triggered by page front matter, like;

---
title: Rule
fixable: true
recommended: false
removed: true
---

Rule docs here

So for existing rules, we can create that front matter, and for removed rules we can just define it manually (maybe just store a data file with that info?).

@pedrottimark
Copy link
Member

👍 So the eslint/eslint/docs/rule/*.md files will have consistent structure whether active or removed!

@nzakas
Copy link
Member

nzakas commented May 20, 2016

Can't we simplify this info?

docs: {
  replacedBy: ['no-confusing-arrow', 'no-constant-condition'],
  deprecated: "Some text describing why the rule was removed"
}

I don't think the date is useful or practical for this purpose (manually including dates is notoriously inaccurate). Since no further changes so be made to the file, you can just look at the last commit with a change to this file to get the date when necessary.

@pedrottimark
Copy link
Member

pedrottimark commented May 20, 2016

Yes we can. In #5774 (comment), I omitted the following information because it is already available:

  • ./conf/replacements.json has the Replaced by rules
  • ./versions.json has the ESLint versions in which rule was added and removed

@nzakas
Copy link
Member

nzakas commented May 20, 2016

I think replacements.json is only updated when a rule is removed, so the info is best kept in a deprecated rule until that point.

@platinumazure
Copy link
Member

@nzakas The reason I suggested a datetime is because we could actually handle expirations in that way. (I don't think we need a deprecation datetime, but rather an expiration datetime.) That way, we could note in the documentation when we plan on removing a rule. (Presumably that would be a year out from deprecation, per the other discussion on rule deprecation/removal.) But perhaps that's not as useful in your mind as I think it might be?

@pedrottimark
Copy link
Member

Any thoughts whether it communicates or confuses in the doc for a removed rule:

  • To display recommended icon if the rule was recommended at the time it was removed. If yes, I will verify that the 3 I think might have been really were.
  • To display recommended (if yes to above) and fixable icons in gray to reinforce past tense “enabled” and “fixed” in the sentences.

5774 removed rule gray icons

@nzakas
Copy link
Member

nzakas commented May 24, 2016

@platinumazure as I mentioned, we can get the date time from the git log whenever we want. I'd prefer not relying on human-provided dates, which can easily be mistyped or otherwise incorrect.

@pedrottimark if a rule was removed, then i think we shouldn't show anything about it being recommended.

@kaicataldo kaicataldo added the help wanted The team would welcome a contribution from the community for this issue label Dec 23, 2016
@wkurniawan07
Copy link
Contributor

wkurniawan07 commented Feb 15, 2017

Hello! I'm a newbie and would like to work on this issue, is it still open valid? 🙂

@platinumazure
Copy link
Member

@wkurniawan07 I see that we don't show any "recommended" info on individual rule pages, but we do show "fixable" info on individual rule pages. I think this is probably valid, unless the team decides we don't want it.

@eslint/eslint-team Do we want to augment the single-rule documentation template to show "recommended" similar to how we show "fixable" (with an icon and a small paragraph of text)? If not, should we close this issue?

@wkurniawan07
Copy link
Contributor

Thanks for the amazingly quick response!

The plan I have in mind is not to manually add the said line to each of the recommended rule, but rather to read the rule metadata and determine if the rule is recommended or not, and automatically append the said line during the docs generation process if the rule is indeed recommended. If the team decides that "recommended" may not be necessary, at least "fixable" will benefit the same from the proposed enhancement.

I have scanned through the docs generation process and have high confidence that it is possible. 🙂

@platinumazure
Copy link
Member

@wkurniawan07 Ah, now I see what you mean. Yes, it would be nice to be able to autogenerate that if possible.

Maybe you could open a PR against eslint.github.io with your thoughts on how it could be done? (If it won't take a lot of time, I mean.)

@wkurniawan07
Copy link
Contributor

@platinumazure the change to be done will involve this file, so I believe this repository is the right place to put my work on. I will explore and report back either with a PR or a white flag 🏳️ by this weekend. 🙂

@ilyavolodin
Copy link
Member

@wkurniawan07 That's correct. The change should be done in this repository. I'm 👍 for adding both fixable and recommended to docs generation if that can be done easily. I think it should be possible since we always add "fixable" to the third line in the file.

@wkurniawan07
Copy link
Contributor

@platinumazure @ilyavolodin sorry for an update later than promised! Everything's going well, a PR can be expected within one hour or two. 🙂

@wkurniawan07
Copy link
Contributor

wkurniawan07 commented Feb 21, 2017

Hmmm I hit something. There are rules that are marked as "The --fix option on the command line automatically fixes some instances of problems reported by this rule". How should I go about treating this class of rule (only two rules fall into this, if it helps)?

One more thing: how about the --fix options for removed rules (there are three of such rules)? Auto-generation clearly won't be able to detect such.

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Feb 21, 2017

@wkurniawan07 I think it would be fine to just use the same text for all autofixable rules. Actually, a significant number of autofixable rules have edge-cases where they decline to fix a problem (not just the two rules that use the "some instances of problems" message). Maybe we could add the "some instances of problems" message to all autofixable rules, since that makes it clearer that rules aren't providing any guarantees about being able to fix something.

@ilyavolodin
Copy link
Member

Agree. We should use the same message everywhere. If we don't think that message is clear enough, we can always tweak it, since it's going to be a single line change.

wkurniawan07 added a commit to wkurniawan07/eslint that referenced this issue Feb 22, 2017
wkurniawan07 added a commit to wkurniawan07/eslint that referenced this issue Feb 22, 2017
wkurniawan07 added a commit to wkurniawan07/eslint that referenced this issue Feb 22, 2017
wkurniawan07 added a commit to wkurniawan07/eslint that referenced this issue Feb 22, 2017
wkurniawan07 added a commit to wkurniawan07/eslint that referenced this issue Feb 23, 2017
wkurniawan07 added a commit to wkurniawan07/eslint that referenced this issue Feb 23, 2017
@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
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 enhancement This change enhances an existing feature of ESLint help wanted The team would welcome a contribution from the community for this issue rule Relates to ESLint's core rules
Projects
None yet