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: Configure Prettier for docs #17737

Closed
wants to merge 18 commits into from
Closed

docs: Configure Prettier for docs #17737

wants to merge 18 commits into from

Conversation

Zamiell
Copy link
Contributor

@Zamiell Zamiell commented Nov 8, 2023

PR for #17504

@eslint-github-bot
Copy link

Hi @Zamiell!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.
  • The first letter of the tag should be in lowercase

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

Copy link

netlify bot commented Nov 8, 2023

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 6753b7c
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6582fa5e35958c000869baec

@Zamiell Zamiell mentioned this pull request Nov 8, 2023
1 task
@nzakas nzakas changed the title Prettier chore: Configure Prettier for docs Nov 9, 2023
@eslint-github-bot eslint-github-bot bot added the chore This change is not user-facing label Nov 9, 2023
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.

Overall this looks pretty good to me. Aside from the few files we should ignore, it would also be helpful to add a precommit hook to run Prettier on these files.

CHANGELOG.md Outdated Show resolved Hide resolved
docs/src/library/rule.md Outdated Show resolved Hide resolved
docs/src/pages/rules.md Outdated Show resolved Hide resolved
@Zamiell Zamiell changed the title chore: Configure Prettier for docs docs: Configure Prettier for docs Nov 9, 2023
@eslint-github-bot eslint-github-bot bot added the documentation Relates to ESLint's documentation label Nov 9, 2023
@Zamiell
Copy link
Contributor Author

Zamiell commented Nov 9, 2023

not a fan of pre-commit hooks myself.
it is laggy for developers and produces pain, especially when making lots of nice, small commits.
a pre-commit hook is nice in the situation where one developer is pushing directly to a "main" branch without any other checks and balances.
but for ESLint, and any other relatively big project, most code is coming in to the project through pull requests, and a pull request will never be merged if CI isn't passing. (and Prettier should be enforced in CI!)
i find a much better workflow is to enforce Prettier in CI, and then additionally configure the repo settings.json file for vscode to have it automatically summon prettier every time a file is saved.
this is essentially lag free and is the shortest possible distance between type-time and notice-error-time.
(noticing at commit-time with a pre-commit-hook is a midway point between noticing at save-time and noticing at PR-time with CI, but comes with an nasty lag cost)

@Zamiell
Copy link
Contributor Author

Zamiell commented Nov 10, 2023

I added the requested exceptions to the .prettierignore file: 625b360

Copy link
Member

@kecrily kecrily left a comment

Choose a reason for hiding this comment

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

I'm not sure this is necessary, as of right now I think all these changes can be detected and fixed by the markdown-lint we are using

@Zamiell
Copy link
Contributor Author

Zamiell commented Nov 10, 2023

Using markdown-lint as a formatter is a bad idea for the exact same reasons that ESLint itself is deprecating its formatting rules!

CONTRIBUTING.md Outdated Show resolved Hide resolved
@nzakas
Copy link
Member

nzakas commented Nov 13, 2023

We already run precommit hooks for most ESLint files, so let's keep it consistent and do it for Prettier, too.

Copy link

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Nov 23, 2023
@Rec0iL99
Copy link
Member

Ping @Zamiell

@Rec0iL99 Rec0iL99 removed the Stale label Nov 24, 2023
@Zamiell
Copy link
Contributor Author

Zamiell commented Nov 28, 2023

Ok, I added the pre-commit hook and fixed the list tab issue, the PR diff looks a lot cleaner now.

Should I convert this from a draft?

@Zamiell
Copy link
Contributor Author

Zamiell commented Nov 28, 2023

Looks like ESLint is throwing a spurious error related to this PR:

/home/runner/work/eslint/eslint/prettier.config.mjs
  33:1  error  Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

That's a bug in ESLint because an mjs file extension doesn't respect 'sourceType: module' at all. Does anyone know if there's already an issue opened about that?

@mdjermanovic
Copy link
Member

That's a bug in ESLint because an mjs file extension doesn't respect 'sourceType: module' at all. Does anyone know if there's already an issue opened about that?

It's not a bug, our eslint config for this project is just set to treat all files as CommonJS. Can the Prettier config file be a CommonJS .js file?

@Zamiell
Copy link
Contributor Author

Zamiell commented Nov 28, 2023

It's not a bug, our eslint config for this project is just set to treat all files as CommonJS.

then that's a bug in the eslint config for this project and should probably be fixed =p

Can the Prettier config file be a CommonJS .js file?

done

Copy link

github-actions bot commented Dec 8, 2023

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Dec 8, 2023
@fasttime fasttime removed the Stale label Dec 9, 2023
Copy link

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Dec 19, 2023
@kecrily
Copy link
Member

kecrily commented Dec 20, 2023

@Zamiell Is it ready for review now?

@kecrily kecrily removed the Stale label Dec 20, 2023
@Zamiell
Copy link
Contributor Author

Zamiell commented Dec 20, 2023

@kecrily it has been ready to review since this comment

Copy link
Member

@kecrily kecrily left a comment

Choose a reason for hiding this comment

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

There's a big problem with indent. This is caused by the prettier bug mentioned above. prettier/prettier#5019

tabWidth: 2 doesn't fix it, it just goes from one problem to another. We are blocked.

package.json Outdated Show resolved Hide resolved
1. **Features** - new functionality that will aid users in the future.
1. **Enhancements** - requested improvements for existing functionality.
1. **Other** - anything else.
1. **Bugs** - problems with the project are actively affecting users. We want to get these resolved as quickly as possible.
Copy link
Member

Choose a reason for hiding this comment

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

The indent here has only three spaces.

text: string
}
```
```typescript
Copy link
Member

Choose a reason for hiding this comment

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

here too

package.json Outdated Show resolved Hide resolved
Co-authored-by: Percy Ma <kecrily@gmail.com>
Copy link

github-actions bot commented Jan 6, 2024

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Jan 6, 2024
Copy link

This pull request was auto-closed due to inactivity. While we wish we could keep working on every request, we unfortunately don't have the bandwidth to continue here and need to focus on other things. You can resubmit this pull request if you would like to continue working on it.

@github-actions github-actions bot closed this Jan 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore This change is not user-facing documentation Relates to ESLint's documentation Stale
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

6 participants