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: Access to options in processors #29

Merged
merged 3 commits into from
Aug 6, 2019
Merged

New: Access to options in processors #29

merged 3 commits into from
Aug 6, 2019

Conversation

Conduitry
Copy link
Contributor

@Conduitry Conduitry commented Jul 1, 2019

Summary

This would add the ability to send options to processors, via a new processorOptions object in the .eslintrc configuration.

Related Issues

https://gitter.im/eslint/eslint?at=5d0e804ad4535e477a829360

Rendered RFC

Here

WIP Implementation

Here

@jsf-clabot
Copy link

jsf-clabot commented Jul 1, 2019

CLA assistant check
All committers have signed the CLA.

@Conduitry Conduitry changed the title Access to shared settings in processors New: Access to shared settings in processors Jul 1, 2019
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.

I like this proposal and have no objections at this point. (Leaving as a comment to reduce chance of accidental early merge, but I'll come back and approve later.)

I think it makes sense to pass just the settings, but it might be best to wrap them in an object in case we want to add even more logic to processors in the future- better to have one options object which includes options.settings (and whatever else in future) rather than continuing to add to number of positional arguments. Just my two cents.

Thanks for putting this together!

@Conduitry
Copy link
Contributor Author

I like the idea of an options object containing just { settings } for now. I'm not sure what the process is here. Is this the sort of thing I would edit into my PR now, or should we wait for more people to weigh in?

@platinumazure
Copy link
Member

Is this the sort of thing I would edit into my PR now, or should we wait for more people to weigh in?

I would say it's your call. Feel free to edit it in if you think it's likely nobody will push back or come up with something better. 😄

The RFC should definitely evolve in response to feedback over time. This is a process that will take at least a few weeks, so we greatly appreciate your patience!

@mysticatea
Copy link
Member

Thank you for your contribution.

The options for processors sounds good to me.

I have some suggestions:

  • I think that adding processorOptions property is more proper than reusing settings property. Because we have processor property since ESLint 6.0.0, it will be similar to the pair of parser and parserOptions. Symmetry gives us good predictability.
  • Would you add an example in the Detailed Design section? It would help to understand this proposal.
  • For the Documentation section, I think that we need two updates for both plugin authors and users.

@not-an-aardvark

This comment has been minimized.

@mysticatea

This comment has been minimized.

@mysticatea mysticatea added enhancement New feature or request Initial Commenting This RFC is in the initial feedback stage and removed triage labels Jul 3, 2019
@not-an-aardvark

This comment has been minimized.

@Conduitry
Copy link
Contributor Author

Conduitry commented Jul 3, 2019

Oh, I like processorOptions. That seems tidier, and it makes sense not to cram these in with the other settings.

@Conduitry Conduitry changed the title New: Access to shared settings in processors New: Access to options in processors Jul 3, 2019

## Open Questions

~~Do we want to provide the entire configuration object to the pre- and postprocessors, or just the shared settings?~~ Now that we're going to have configuration that's explicitly intended for this processor, it probably makes sense to only send that object.
Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to just provide the whole config object to both pre- and postprocessors. They work as a pair and shouldn't require any isolation.


I can certainly give implementing this a shot, but I've only dug into the ESLint codebase to the extent necessary to work around that lack of this feature. I'd appreciate some help, particularly with writing tests.

If there are other places in the code besides `Linter.prototype._verifyWithProcessor` where processors are run, I'd also like to hear about them.
Copy link
Member

Choose a reason for hiding this comment

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

That should be the only place. Processors are pretty isolated from the rest of the code, as they have been a late addition.

@ilyavolodin
Copy link
Member

It looks like this RFC has been open for over 21 days required for Initial Commenting stage, and there doesn't seem to be disagreements on the content of the RFC, so I'm going to move it into Final Commenting stage.

@ilyavolodin ilyavolodin added Final Commenting This RFC is in the final week of commenting and removed Initial Commenting This RFC is in the initial feedback stage labels Jul 23, 2019
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.

I like the idea of having a processorOptions object rather than sending the whole config. It is consistent with parserOptions and gives plugin authors the opportunity to design a clean interface. Also, I can't see any strong need for processors to access the rest of configuration (though I would love to see a counterexample).

@kaicataldo
Copy link
Member

I think it makes sense to start by limiting processors access to just processorOptions to begin with, with the understanding that we can add more in without it being a breaking change. That would be more consistent with parserOptions (where we do add pass in a few more configuration keys, including ecmaFeatures, ecmaVersion, and enabled envs).

@Conduitry
Copy link
Contributor Author

Anything else needed before this is merged?

As mentioned in the OP, I do have an implementation of this in a fork that seems to be working. I plan on submitting a PR for this. Is there any avenue in particular that I should be asking for help with stuff like writing tests for this?

@platinumazure
Copy link
Member

platinumazure commented Aug 1, 2019

@Conduitry

Anything else needed before this is merged?

Probably not, but I'm not sure. This has been in the final commenting period for 7 days and I haven't seen any explicit objections in that time.

In particular, we don't need to have an implementation PR up before we merge this.

@ilyavolodin Do you have any concerns with creating a processorOptions configuration key, rather than sending in the whole config?

Is there any avenue in particular that I should be asking for help with stuff like writing tests for this?

I don't think there's anything special here. You could reach out on our Gitter chat or ask questions in the implementation PR itself, depending on how far we are in implementation. Hope this helps!

@ilyavolodin
Copy link
Member

ilyavolodin commented Aug 2, 2019

@ilyavolodin Do you have any concerns with creating a processorOptions configuration key, rather than sending in the whole config?

No I think that's fine. My comment was about sending the same information to pre/post processors, not specifically about the whole config.

@platinumazure
Copy link
Member

My comment was about sending the same information to pre/post processors, not specifically about the whole config.

Sorry for misunderstanding. I agree we should send the same information to both preprocessors and postprocessors.

Anything else we need before merging this?

@mysticatea mysticatea merged commit 911e1d0 into eslint:master Aug 6, 2019
@mysticatea
Copy link
Member

I think that this RFC is good to finish. Thank you very much, @Conduitry !

@Conduitry Conduitry deleted the 2019-processor-shared-settings branch August 6, 2019 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Final Commenting This RFC is in the final week of commenting
Projects
None yet
7 participants