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

New: Implement processorOptions #12068

Closed
wants to merge 10 commits into from

Conversation

Conduitry
Copy link

What is the purpose of this pull request? (put an "X" next to 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
[ ] Other, please explain:

What changes did you make? (Give an overview)

This implements processorOptions, as described in this RFC.

This isn't ready yet, and I'm marking this PR as a draft. This is working for me locally, but I'd like to get more eyes on this now that the RFC has been merged.

TODO:

  • Tests
  • Documentation

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

Tests, mostly. I'd appreciate some pointers about where tests should go and what I should look at.

I'd also like confirmation that I've updated the type comments appropriately.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Aug 6, 2019
@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint and removed triage An ESLint team member will look at this issue soon labels Aug 6, 2019
@Conduitry Conduitry marked this pull request as ready for review September 2, 2019 12:08
@Conduitry
Copy link
Author

I'm marking this as ready for review.

I've added some minimal docs for this. I'm not sure what else is wanted here.

I also have some questions about what sorts of tests should be added and where they should live. Should they focus on config extraction? Something else?

Thanks!

lib/linter/linter.js Outdated Show resolved Hide resolved
@mysticatea
Copy link
Member

About tests, we should add the tests of user-facing behaviors. I think there are two.

  • One is that CLIEngine#executeOnFiles(patterns) should pass processorOptions to preprocess and postprocess. (for a reference, some tests for processors are there: tests/lib/cli-engine/cli-engine.js#L3033)

And, sorry that I had overlooked while writing RFC, processorOptions should be in options in Linter#verify(code, config, options).
Because Linter.verify(code, config, options) doesn't support some properties of config (config.extends, config.plugins, config.overrides, and config.processor). About processors, instead, we use options.preprocess and options.postprocess in a historical reason (document is https://eslint.org/docs/developer-guide/nodejs-api#linterverify). Therefore, I feel odd if config.processor is not supported, but config.processorOptions is supported.

So...

  • The other one is that Linter#verify(code, config, options) should pass options.processorOptions to options.preprocess and options.postprocess. (for a reference, some tests for processors are there: tests/lib/linter/linter.js#L4325)

@Conduitry
Copy link
Author

Should config.processorOptions or options.processorOptions take precedence? Should they be merged in some way? Or is it a problem if config.processorOptions is supported at all in Linter#verify?

@mysticatea
Copy link
Member

I think config.processorOptions should be ignored completely because config.processor is not supported in Linter#verify.

@mysticatea
Copy link
Member

Hi. The implementation looks good to me.

Would you add some tests I mentioned in #12068 (comment) ? If you need helps, let tell me!

@Conduitry
Copy link
Author

Hi, sorry about the big delay on this! I'll try to add some tests within the next day and will let you know if I get confused about something.

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Adding a "request changes" review just to avoid accidental merge. @Conduitry Please feel free to ping me when you've added some tests and feel this is ready for review. Thanks!

@mysticatea mysticatea added this to Implemented, pending review in RFCs Dec 8, 2019
@kaicataldo
Copy link
Member

@Conduitry Friendly ping! We'd love to merge this.

@Conduitry
Copy link
Author

Hey! Sorry again about the even bigger delay. I'm taking a look at this again. I'll let you know if I'd like some more guidance on specifically what sort of tests I should be writing for this.

I was also looking into why the tests are currently failing but it looks like that's just #12662.

@nzakas
Copy link
Member

nzakas commented Feb 25, 2020

@Conduitry are you still planning on working on this?

@kaicataldo
Copy link
Member

@Conduitry I'm going to close this so that others feel free to contribute. Please don't hesitate to reopen if/when you have the time!

@kaicataldo kaicataldo closed this Jun 5, 2020
@btmills btmills moved this from Implemented, pending review to Accepted, ready to implement in RFCs Aug 27, 2020
@btmills btmills removed this from Accepted, ready to implement in RFCs Aug 27, 2020
@JamesHenry
Copy link
Member

@kaicataldo so just to confirm, the status for this one is that the RFC remains approved and the team will accept a new implementation PR from anybody at any time? (I have a use-case for this within angular-eslint)

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo reopened this Oct 14, 2020
@kaicataldo kaicataldo closed this Oct 14, 2020
@kaicataldo
Copy link
Member

Whoops, meant to just comment, not reopen 😅

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Apr 13, 2021
@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 Apr 13, 2021
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 core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants