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

Custom formatting of existing rules #10705

Closed
chrissinclairedited opened this issue Jul 30, 2018 · 5 comments
Closed

Custom formatting of existing rules #10705

chrissinclairedited opened this issue Jul 30, 2018 · 5 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue 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 feature This change adds a new feature to ESLint

Comments

@chrissinclairedited
Copy link

The version of ESLint you are using.
3.5.0

The problem you want to solve.
We're currently investigating the ability to embed a link in eslint messages that would point to our own internal discussions and justifications for particular rules & config, along with examples of best practice specific to our codebase. This is primarily aimed at helping new developers on the team get up to speed with the rules more easily.

As far as I can see eslint would currently require us to write a custom formatter. That's probably achievable, but really we're after something more lightweight that would just customise the message the rule produces, and is compatible with existing formatters, particularly those that provide IDE integration.

Your take on the correct solution to problem.
Ideally we'd be able to provide a custom function as part of our eslint config that would be given each message (along with some available context, e.g. the rule that triggered the message), which would be required to return a new string, which would become the new message. I'm not familiar with the implementation of the rules, and if each individual message is still available after the formatter has done it's work. An alternative would be to require each formatter to call this new customising function. That would however require each formatter to be updated to use this hook.

Thanks in advance!

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jul 30, 2018
@not-an-aardvark
Copy link
Member

Hi, thanks for creating an issue. A few thoughts:

  • It might be possible to create a new formatter that's just a wrapper around existing formatters (i.e. it takes the results object, modifies the messages, and invokes another built-in formatter on the transformed results). This would probably be simpler than writing a complete formatter from scratch. However, a formatter solution might not work well with IDE integrations, because they typically obtain output from ESLint directly without passing the output through a formatter.
  • It would also be possible to use eslint-rule-composer to transform messages from rules, by effectively creating new rules and configuring those instead. However, this might increase the complexity of configuring those rules, particularly when using shareable configs (which would need to be aware of the new rule names).
  • This could potentially be solved in a different way if we implement an API for message internationalization (Proposal: locale option and meta.messages revision #9870).

Configs are currently static data (while we allow people to use .eslintrc.js files, they don't currently have any core features that aren't available to static config files), so my gut reaction is that we might not want to add features to a config file that require code execution, such as a transformer function. But I'm open to looking at other potential solutions that would avoid the problem.

@chrissinclairedited
Copy link
Author

Hi, thanks for your response!

I'd not spotted eslint-rule-composer, but otherwise I'd agree with your points 1) and 2). Really I'd like to keep the IDE integration if possible, which limits how much 1) can be used, although it does feel like the simplest way to achieve this otherwise.

I'm not really a fan of 2) - given that we really want to help new developers (particularly those new to developing systems generally) get up to speed with the reasoning behind the rules I'd rather avoid having to then make them parse a different rule name that's not widely used. The overhead of maintaining custom versions of every rule we use just to add a link to the message seems high.

3) Sounds like it could be used to accommodate this, depending on the exact implementation. I think for us it would be useful if we were able to override the default locale to point at our messages file / function. Presumably we'd be able to do that in our .eslintrc. I don't see us needing to provide localised versions of our transformed rules, so a two-step translation probably wouldn't be needed (although that may be useful for others, I'm not clear on that. My initial thought is the complexity of managing something like that would be too high).

Having read the linked thread #9870 it sounds like there's a few issues to be worked through before that lands. FWIW I'd be happy to implement something on a fork and then share our experiences of it, if you'd be open to providing a bit of a steer here and there.

I also agree with your point about executable config - I imagine this would have to take the form of a path to a local module that exports the transformer function (this could potentially be a separate module from node_modules, if it was just require'd, which would allow a separate ecosystem to build up around that as needed. I'm not clear if there'd be much take up on that - I appreciate our use case is somewhat specific!). If this was done through the i18n route then that would have to be taken in combination with CLI flags etc.

@aladdin-add aladdin-add added core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint 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 Aug 1, 2018
@betaorbust
Copy link
Contributor

I had this exact need -- setting personalized context on rules and adding links to project-specific reasons why a rule was turned on and how to fix it.

My solution was:

  • Split up the "message" concept (a single string) from the "context" concept (an array of links to docs)
  • Externalize the definitions of new messages and contexts from the ESLint formatter (allow us to use our own defs instead of being tied to the specific formatter)
  • Make the idea of a "compassionate formatter" which can combine the message+contexts overrides into an ESLint-compatible formatter that has all the message overrides and contexts applied.

The results are https://www.npmjs.com/package/eslint-formatter-compassion
image

It's not a perfect solution for a couple reasons:

  • It's only per-rule-id instead of per-rule-id + per-message-id which limits how much you can target the output. This is because message IDs are not supplied by ESLint to the formatter.
  • Because fomatters don't have first-party support for configuation (per @nzakas in Formatter options #2989 😭), you have to essentially bake out the new formatter instead of just being able to hand "use this formatter and these guides"
  • It's limited to the ESLint formatters I've reworked into compassionate formatters (stylish, codeframe, and visualstuido)

That said, being able to set team-based context, shortcutting the overhead for a failure etc. is a huge win for my teams.

If there was a long-term ask from ESLint, it would be

  • Support rules publishing their own "context" links. Most of these would be essentially whatever's written in the doc part of the meta section, but allowing it to be set at report would be even better for granularity and helping engineers get to the answers they need when the run into an error/warning.
  • Surface the message ID in the reporting structure along with the rule ID for better targeting.
  • Support a formatter config option.
  • Allow an option to load in message/context overrides based on rule ID or rule ID + message ID.
    • I've put together a demo using eslint-formatter-compassion which handles i18n via this method and, while it's cumbersome as an external interface to do this, it would address a number of the i18n issue as you're down to making messages from enums and data at that point.

A nice thing is that this problem gets much much simpler even if we only go with a few of those asks.

@betaorbust
Copy link
Contributor

Also, this would go hand-in-hand with #9870 because if both were implemented, rules that didn't have language support in a certain language could be augmented by third-parties in a blessed-path fashion while they build up the body of work.

@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Dec 11, 2018
@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jun 10, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue 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 feature This change adds a new feature to ESLint
Projects
None yet
Development

No branches or pull requests

4 participants