Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

Add warning for suggested changes to rule docs #899

Open
btmills opened this issue Dec 28, 2021 · 10 comments
Open

Add warning for suggested changes to rule docs #899

btmills opened this issue Dec 28, 2021 · 10 comments
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion documentation Relates to ESLint's documentation
Projects

Comments

@btmills
Copy link
Member

btmills commented Dec 28, 2021

In the 2021-12-16 TSC meeting, we discussed feat: Fixer for missing unicode flag in no-misleading-character-class. That PR adds an editor suggestion that will add the u flag. Sometimes, adding that flag may change behavior of other portions of the regular expression unrelated to the misleading character class. While adding the u flag is probably the best fix most of the time, we're implementing the change as a suggestion rather than an autofix because we can't be sure the indirect behavior changes are correct.

More generally, we want to make sure users know that they should consider all possible implications of the behavior change before applying editor suggestions. Currently, documentation pages for rules with suggestions (like no-unsafe-negation) include a light bulb icon and the text "Some problems reported by this rule are manually fixable by editor suggestions."

In the meeting, we agreed to change the wording to "Some problems reported by this rule are manually may be fixable by editor suggestions. (Warning)" and link "Warning" to a page that describes the potential pitfalls.

@btmills btmills added accepted There is consensus among the team that this change meets the criteria for inclusion documentation Relates to ESLint's documentation labels Dec 28, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Dec 28, 2021
@btmills btmills moved this from Needs Triage to Ready to Implement in Triage Dec 28, 2021
@pranay101
Copy link

Hi, I am new to open source and I would like to work on this issue. Please guide me further on what needs to be done.

@nzakas
Copy link
Member

nzakas commented Jan 8, 2022

@pranay101 thanks for the interest! We would need to add a link here:
https://github.com/eslint/website/blob/a9e5dbf11025f05d2717a8d855772b55fd070ed9/docs/rules/index.liquid#L10

And then add a page that it links to describing what suggestions are and how they might produce changes that alter the code behavior.

@pranay101
Copy link

hey @nzakas which link would you like me to redirect the users to?

@nzakas
Copy link
Member

nzakas commented Jan 25, 2022

We can probably start with /user-guide/fixes-and-suggestions. Note that we would also need to add this into the main eslint repo in the docs folder (which is the source of truth for the website)

@AkashaRojee
Copy link
Contributor

Hello!

Is this issue already being worked on, please? If not, then I would like to take it.

Thanks!

@nzakas
Copy link
Member

nzakas commented Feb 18, 2022

@AkashaRojee you are free to work on it 👍

AkashaRojee added a commit to AkashaRojee/eslint-website that referenced this issue Feb 20, 2022
Created /fixes-and-suggestions/no-unsafe-negation.md under /user-guide,
and added fixes and suggestions using information from the rule's test source.

This file is a first draft to verify the content.
Once approved, it will be added to the main eslint repo,
and future PRs for this file will be accepted only from there.

There are also parts that could be removed,
and instead generated through the main eslint repo's Makefile.
Some of these parts are:

- front matter
- comment for non-native file
- Version section
- Resources section

Refs eslint#899
@AkashaRojee
Copy link
Contributor

AkashaRojee commented Feb 20, 2022

@nzakas

Great!

I worked on a first draft of the fixes and suggestions for no-unsafe-negation.

Please, find it here.

I have a few questions about the next steps. Once they are confirmed, I will proceed to the fixes and suggestions for the other rules.

1. Is the content in the first draft correct?

I used the information from the rule's test source.

Is there anything I should add/change?

2. Which folder/file structure is preferred?

Option 1: /user-guide/fixes-and-suggestions.md, with a section for each rule

Option 2: /user-guide/fixes-and-suggestions/rule-name.md, for each rule (and an index.liquid page under /fixes-and-suggestions)

Depending on the above, I will then add the correct fixes-and-suggestions link to the following, as you mentionned above.

https://github.com/eslint/website/blob/a9e5dbf11025f05d2717a8d855772b55fd070ed9/docs/rules/index.liquid#L10

3. How to proceed with the PRs for the website repo and the main repo?

I understand that we would also need to add this content into the main repo in /docs which is the source of the truth for the website. Do I create a PR for the file both in the website repo and the main repo?

While inspecting the main repo, I saw that a Makefile is used to automate the generation of parts of the documentation, e.g.:

  • the front matter
  • the comment for non-native files
  • the Version section
  • the Resources section

Should I remove these sections from the file, and modify the Makefile accordingly?

Thanks!

@nzakas
Copy link
Member

nzakas commented Feb 23, 2022

Thanks so much for working on this. I think we may have a misunderstanding here: we aren’t looking for individual updates to rules where we list what happens with each suggestion. All we are doing here is adding a link from the rules index page that explains suggestions in all rules may not be safe.

As such, option 1 is the best choice to display a generic message about why suggestions may not be safe.

Regarding where to make changes: the content of the warning should be in the eslint repo. There is no need to make changes to the Makefile. You can then add a link to the rules index in the website repo. We will wait to merge that until the next release, when the new content from the eslint repo is copied over.

@AkashaRojee
Copy link
Contributor

@nzakas Thanks for the clarification. Sorry for the delay, I was out of action. I'll get to it this week 🚀

@nzakas
Copy link
Member

nzakas commented Mar 9, 2022

No problem, thanks for the followup.

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 documentation Relates to ESLint's documentation
Projects
No open projects
Triage
Ready to Implement
Development

No branches or pull requests

4 participants