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
Conversation
|
There was a problem hiding this 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.
I'll start working on this soon when I get off work |
Test was added
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 |
Please add the docs in this PR. We release features and docs at the same time. |
Docs are added, and I used |
Some help is needed, I'd like to modify the Line 37 in a1f7ad7
|
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done @mdjermanovic
There was a problem hiding this comment.
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.
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 It would be also very nice to update this jsdoc: Lines 627 to 634 in c9fefd2
|
Thanks for the tips and suggestion! |
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 |
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
There was a problem hiding this 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.
Thanks for contributing! |
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. |
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. ❤️ |
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 offormatter.format
Is there anything you'd like reviewers to focus on?