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

feat: support async formatter #15243

Merged
merged 12 commits into from Nov 24, 2021
Merged

feat: support async formatter #15243

merged 12 commits into from Nov 24, 2021

Conversation

fengzilong
Copy link
Contributor

@fengzilong fengzilong commented Nov 2, 2021

closes #15242

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
[x] Add something to the core: support async formatter
[ ] Other, please explain

What changes did you make? (Give an overview)

Added await ahead of formatter.format

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

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 2, 2021

CLA Signed

The committers are authorized under a signed CLA.

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Nov 2, 2021
@nzakas nzakas added core Relates to ESLint's core APIs and features 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 Nov 3, 2021
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.

Thanks for this. We would just need some tests to verify this change works as intended.

We would also need the formatter documentation to be updated.

@fengzilong
Copy link
Contributor Author

We would just need some tests to verify this change works as intended.
We would also need the formatter documentation to be updated.

I'll start working on this soon when I get off work

@fengzilong
Copy link
Contributor Author

fengzilong commented Nov 3, 2021

We would just need some tests to verify this change works as intended.

Test was added

We would also need the formatter documentation to be updated.

I'm not sure in which version will ESLint support async formatter. I think I'd better add documentation after the releasing, or it maybe mislead some formatter developers( I will add documentation then in another PR, and provide the version number in docs ). Hope to hear your thoughts

ping~ @nzakas

@nzakas
Copy link
Member

nzakas commented Nov 4, 2021

Please add the docs in this PR. We release features and docs at the same time.

@fengzilong
Copy link
Contributor Author

fengzilong commented Nov 4, 2021

Please add the docs in this PR. We release features and docs at the same time.

Docs are added, and I used from ESLint 8.2.0 in the documentation, not sure whether it is the right version number for next releasing, please help review this, thanks~

@fengzilong
Copy link
Contributor Author

fengzilong commented Nov 4, 2021

Some help is needed, I'd like to modify the Formatter type definition, but I just couldn't find where it is from, the ./load-formatter is missing in codebase.

/** @typedef {import("./load-formatter").Formatter} Formatter */

tests/lib/cli.js Outdated
@@ -287,6 +287,17 @@ describe("cli", () => {
});
});

describe("when given an async formatter path", () => {
it("should execute without any errors", async () => {
const formatterPath = getFixturePath("async-formatter.js");
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
const formatterPath = getFixturePath("async-formatter.js");
const formatterPath = getFixturePath("formatters", "async-formatter.js");

Can we move this file to fixtures/formatters/, where the other formatters are?

Copy link
Contributor Author

@fengzilong fengzilong Nov 6, 2021

Choose a reason for hiding this comment

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

I did it before, but putting that file to fixtures/formatters/ made other tests fail, not sure why. I'll try it again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

It appears that we're using files in fixtures/formatters/ as source code files in some tests, so whenever we add a fixture formatter to test how the formatters feature works, we have to update some unrelated tests that are linting those files.

I'll fix this in a follow-up.

@mdjermanovic
Copy link
Member

Some help is needed, I'd like to modify the Formatter type definition, but I just couldn't find where it is from, the ./load-formatter is missing in codebase.

/** @typedef {import("./load-formatter").Formatter} Formatter */

Indeed, it's missing. I'd guess that the original plan was to have a separate module for loading formatters and define the type in that file. I searched for Formatter and couldn't find the type definition anywhere. It would be very nice to define that type here (in eslint.js file, somewhere in the // Typedefs section).

It would be also very nice to update this jsdoc:

eslint/lib/eslint/eslint.js

Lines 627 to 634 in c9fefd2

return {
/**
* The main formatter method.
* @param {LintResults[]} results The lint results to format.
* @returns {string} The formatted lint results.
*/
format(results) {

@fengzilong
Copy link
Contributor Author

Thanks for the tips and suggestion!

@fengzilong
Copy link
Contributor Author

fengzilong commented Nov 7, 2021

Done, and it's my first time to write typedef in jsdoc format, so there may be some mistakes, please help review the changes, thanks

Friendly ping~ @mdjermanovic

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion cli Relates to ESLint's command-line interface enhancement This change enhances an existing feature of ESLint and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Nov 23, 2021
docs/developer-guide/working-with-custom-formatters.md Outdated Show resolved Hide resolved
lib/eslint/eslint.js Outdated Show resolved Hide resolved
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@fengzilong
Copy link
Contributor Author

Updated

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!

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.

LGTM. Thanks!

Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for your contribution.

@mdjermanovic mdjermanovic merged commit f1b7499 into eslint:main Nov 24, 2021
@mdjermanovic
Copy link
Member

Thanks for contributing!

@nzakas
Copy link
Member

nzakas commented Dec 2, 2021

Thanks @fengzilong! We've decided to pay you for this contribution. If you can please send an email to contact (at) eslint (dot) org, we can send you the info.

@fengzilong
Copy link
Contributor Author

fengzilong commented Dec 7, 2021

Sorry I just saw the message. Thanks for your generosity, but it was only a little work for me, so you don't have to pay me. I'm very happy if this would help more people other than me.

ESLint is an awesome project. If there is any chance, I'd like to contribute more to this project. ❤️

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators May 24, 2022
@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 May 24, 2022
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 cli Relates to ESLint's command-line interface core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change Request: Support async formatter
4 participants