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: Programmatic Style Editors #6

Closed
wants to merge 5 commits into from
Closed

New: Programmatic Style Editors #6

wants to merge 5 commits into from

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Dec 11, 2018

Summary

A way to extend the capabilities of ESLint's --fix command to enable applying programmatic style edits instead of (or in addition to) using stylistic rules. Developers can create automatically applied style guides by writing JavaScript code rather than configuring individual rules.

Related Issues

n/a

@nzakas nzakas added the feature label Dec 11, 2018
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.

Left a few comments where some lists' preceding paragraphs don't quite match the lists (as far as I can tell). I didn't leave suggestions because I wasn't sure what the intent was.

Thanks for putting this together!

designs/2018-programmatic-style-edits/README.md Outdated Show resolved Hide resolved
@platinumazure platinumazure dismissed their stale review December 11, 2018 21:33

Concerns were addressed

Copy link
Member

@not-an-aardvark not-an-aardvark 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 working on this RFC. I've left a few comments.


Prettier has also shown that there is a desire to not fill up end-user screens with multiple error messages advising developers to add a space here or remove a space there; they'd much rather just have one command to fix the style of their source code.

Additionally, a growing number of developers use ESLint together with Prettier to get both the error-checking capability of ESLint with the source code formatting of Prettier. While there are some approaches to making these two tools work together, any quick search of Twitter will find new developers tend to struggle to get these two tools to work together.
Copy link
Member

Choose a reason for hiding this comment

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

Could you elaborate on what these usability issues are? The most frequent issue I've seen when people are using eslint-plugin-prettier is that they don't realize they need to disable other stylistic rules. But this seems like it would still be a problem if we introduced style editors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'll add in what I've been seeing. The tl;dr is that the most common ESLint-related tweets I see are how people are having trouble making ESLint work with Prettier.


If people don't specify a style editor, then their experience doesn't change at all.

## Alternatives
Copy link
Member

Choose a reason for hiding this comment

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

Another alternative would be to have users use separate formatting tools after running ESLint (e.g. by having a script that runs ESLint and then formats their code with prettier).

This would avoid the need to add complexity to ESLint core, but as a downside it wouldn't work as well with ESLint processors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I'll add that in.

style: "mystyles/es5",
styleOptions: {
indent: "tabs"
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused about the value-add here. Couldn't the same thing be accomplished by having the mystyles function export an es5 rule, which modifies the entire contents of the file as an autofix?

(I realize that this would work a bit differently in terms of the order of operations, but the UX would largely be the same as far as I can tell.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and in fact, that's basically what a lot of our whitespace-related rules are doing right now.

The value-add, in my mind, is:

  1. There's not a lot of value to printing out a warning about a style problem that can easily be fixed. I know I get really annoyed when I see a "missing space" or "incorrect indentation" warning. I'd rather it just be fixed and not bother me.
  2. The fix API is fairly limited in what it can do right now. As I've mentioned before, it was really designed for making tweaks here and there but we are now using it for all kinds of changes that it wasn't designed for (think function expressions to arrow function expressions). It's really difficult to change how rule fixes work because of the single traversal architecture, which is why we have to apply fixes after linting instead of during. Style edits are realtime changes that basically have no limitations.

Copy link
Member

@not-an-aardvark not-an-aardvark Dec 12, 2018

Choose a reason for hiding this comment

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

Thanks for explaining.

I'm not sure I understand point 2. To me, it seems like the current fix API is difficult to use not because it's limited in expressive power (rules can already replace the entire contents of the file if they want to), but rather because the API operates at a low level of abstraction (it only allows raw text-based replacements rather than AST-based replacements). In principle, rules could create autofixers that use recast for formatting, although I'm not aware of any rules that do this at the moment.

It seems to me that style editors would be similarly low-level in that they would simply provide a text-based API. So I don't see how this would be solving any problems with the current fix API that wouldn't already be solvable by having a rule that does something like this:

context.report({
    /* ... */
    fix: fixer => fixer.replaceTextRange([0, sourceText.length], formatCode(sourceText))
});

Copy link
Member Author

Choose a reason for hiding this comment

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

The goal here is not to workaround limitations of fixers, it's to create a new way to make changes that are always applied (literally a source code formatter). It is not possible right now to say, "I want to make these changes to my source code but don't bug me about the individual tweaks that need to be made." Plus, this allows us to outsource small tweaks to style rules to the end users who want them rather than needing to create more and more options for core rules.

It helps to think about a concrete example: think of the indent rule. Everyone has an opinion about the right way to indent things, and we've tried to keep up by adding options for each preference. What if instead the indent rule just did basic indentation without exceptions, and then anyone who wanted to, for example, outdent case statements could just apply an edit to do that? This would free up a lot of time for the team. We'd be able to say, "we aren't making more changes to this rule, if you need to tweak the output, create a style edit."

I can tell from your questions that I haven't done a good job outlining this motivation in the RFC, so I'll update the motivation section.

@nzakas
Copy link
Member Author

nzakas commented Jan 10, 2019

I've updated the motivation section of this RFC to try to better explain why I think this is an important feature for ESLint. Hopefully it makes sense -- my brain has been a bit foggy, so let me know if there's anything weird in there. :)

@kentcdodds
Copy link

  • The rational is 👍
  • The goals are 👍
  • API is 👍
  • Behavior is 👍

Here are a few comments/thoughts.

And because style editors could potentially introduce linting errors, one additional lint operation must take place before the source code results are reported. This last lint operation does not do any autofixing and only ensures that the final source code is given the correct linting messages.

This is a great idea. Might I ask that in the event a style editor made caused a linting error a helpful error message is printed or some kinda of annotation on the error indicating that the style editor and linting rule conflict. This will make it much less frustrating for people who keep fixing the linting rule then run --fix and it's broken again.

Is the hope that eventually linting rules and fixers be separated so linting rules don't have a fixer and instead fixing happens via a style editor? Just curious. No recommendations either way.


Overall I'm in favor of this change. I don't personally have the bandwidth to help with anything, and I'm honestly pretty happy with the way things are currently (using eslint as a linter-only and prettier as a formatter), but if there were something I could do to have both in one tool that'd be neat. Though I think it's already possible via eslint-plugin-prettier actually. I'm guessing this would just make tools like eslint-plugin-prettier easier to implement.

designs/2018-programmatic-style-edits/README.md Outdated Show resolved Hide resolved
* **They want a change that fits with an existing core rule by no option exists.** In this case, end-users make a request to the ESLint team to enhance an existing rule with new options. The ESLint team then must spend time evaluating the proposal, ensuring consistency of option names across core rules, ensuring no overlap with existing rules, and whether there is someone interested enough to implement the change.
* **They want a change that doesn't fit with an existing core rule.** In this case, there are actually two possible outcomes:
1. The end-user requests a new core rule be added to ESLint. Once again, this brings with it the burden of evaluating, researching, and potentially implementing a new rule.
1. The end-user decides to create a custom rule. Some end-users are happy to do this and distribute their own plugins, however, ESLint limitations such as the inability to easily distribute plugins with shareable configs (https://github.com/eslint/eslint/issues/3458) means that many end-users prefer proposing their own style rules to be included with ESLint. (There is also a bit of halo effect around core rules, where end-users feel that getting their own rules into the ESLint core means they are "blessed" and therefore more valid than custom rules distributed in plugins.)
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 the inconvenience of using custom rules is a significant problem, and I agree that it has a side-effect of increasing the maintenance burden on the ESLint team. Regardless of whether we add style editors, I think it would be a good idea for us to work on improving the ergonomics of custom rules in the future.

That said, it doesn't seem to me that style editors would be any more convenient to use than custom rules, given that style editors are also distributed with plugins. If the user is already creating a plugin, then it's easy enough for them to just add a custom rule to that plugin. The difficulty is that it's inconvenient to depend on plugins (due to eslint/eslint#3458 and the fact that project-specific plugins still need to be in node_modules). So I still think the use case of style editors could already be adequately fulfilled by using custom rules.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point, there is definitely a parallel problem here. I do believe that style editors solve the problem in a different way than rules because you are not required to name, define options for, or otherwise package up individual configurable options. In fact, I suspect in most cases that style editors will not have many, if any, options, because the point of enforcing a style guide is that you don't want people to have options -- the code should be the One True Way.

The problem is when end-users want to customize a style enforcer to fit their specific needs. There are three basic scenarios that play out every time an end-user wants to customize

* **They want a change that is already supported by a core rule.** In this case, end-users can use the documentation to determine the the correct rules and rule options to use to get their desired result. This is the case ESLint was optimized for.
* **They want a change that fits with an existing core rule by no option exists.** In this case, end-users make a request to the ESLint team to enhance an existing rule with new options. The ESLint team then must spend time evaluating the proposal, ensuring consistency of option names across core rules, ensuring no overlap with existing rules, and whether there is someone interested enough to implement the change.
Copy link
Member

Choose a reason for hiding this comment

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

While this is an accurate description of what usually happens, it's worth noting that most rule option suggestions can already be implemented using eslint-rule-composer, including the modification to the indent rule that you discussed in #6 (comment). I think we could decide to stop adding options to stylistic rules without simultaneously adding style editors, because users would still be able to use eslint-rule-composer or custom rules to serve the same use cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I hadn't realized eslint-rule-composer could also add new options, that's very cool!

I agree, we could definitely decide to stop adding new options to stylistic rules and accomplish part of what I'd like to accomplish with this design. The other part, which is that I don't believe configuring 200 individual rules is a good way to define a style guide, would not be solved.


The goals of this programmatic style editor feature are:

1. Allow style enforcers that do not rely on rules.
Copy link
Member

Choose a reason for hiding this comment

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

Could you clarify this goal? If I've understood your position correctly, you're arguing that style editors provide some combination of UX/development experience/composability that isn't possible to achieve with rules. In that case, it seems like the lack of reliance on rules is a detail of the solution rather than a goal, and the goal should be something like "Allow style enforcers with better UX" or "Allow style to be enforced without displaying linting errors to the user" instead. The current wording of the goal would tautologically rule out any UX improvements that just use rules. (It's debatable whether just using rules would be adequate for those UX improvements anyway, but in having that discussion it would be good to agree on the high-level problems we're trying to solve.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm trying to get across that I don't believe style guides (or style editors) should be using individual rules. As I mentioned in a previous comment, the idea that you need to individually configure 200 rules to get the style guide you want seems like more than is reasonable to ask of devs. I'm envisioning a world where ESLint has no stylistic rules and all style editors are created by the individuals who want them using whatever tools they want while still working seamlessly with ESLint. I'll add more about that into the RFC.

Copy link
Member

@kaicataldo kaicataldo Jan 15, 2019

Choose a reason for hiding this comment

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

So, just to clarify, in this proposal, rules are used to enforce non-stylistic patterns and style enforcers are used to "enforce" (autofix) styles? In other words, the ultimate end goal here would be to eventually deprecate all stylistic rules in favor of the new style enforcers?

Edit: Sorry, I missed the section below that says exactly this.

},
edits: [
...airbnbStyle.edits,
require("./edits/indentation")
Copy link
Member

Choose a reason for hiding this comment

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

It seems like style editors would often conflict with each other, similar to how autofixing one rule occasionally creates linting errors for another rule. Autofixes from rules are applied multiple times (through multipass autofixing), which addresses the vast majority of conflicts. Would style editors also be applied multiple times?

Example of a conflict:

module.exports = {
    edits: [
        require("./edits/indentation"),
        require("./edits/add-linebreaks-if-lines-are-too-long")
    ]
};

The add-linebreaks-if-lines-are-too-long style editor wouldn't know the user's indentation preferences, and presumably it wouldn't want to bundle indentation logic, so the user's code would end up with some indentation errors after linebreaks are added. It's likely that the user wouldn't notice this issue because no linting errors would be reported at the end (assuming the user disables the indent rule in favor of just using style editors).

Note that switching the order of the style editors wouldn't help here. If user decided to run the indentation editor after the linebreak-adding editor, then adding indentation to a line could cause it to become "too long" such that it would be appropriate to have linebreaks added, but the linebreak-adding editor would have already run.

More generally, I have some doubts about how composable style editors would be in practice. It's true that the sequence of two valid style editors is also a valid style editor, but it seems like it would be difficult to ensure that the intended aspects of the original style editors are still preserved after combining them.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are correct in that this is possible. The difference from the current situation is that fixing the conflict is solely on the end-user and doesn't involve us (compared to now, where we end up trying to figure out how to get two rules to not conflict).

In the specific case you mention, the options for resolving the problem are:

  1. Choose just one of the edits.
  2. Choose a different order of the edits based on your priority.
  3. Remove both edits and write a new one that does what you want.

The most common scenario in my mind is something like this: let's say there is an indent edit that I've provided in a package. By default it indents a switch block like this:

switch (foo) {
case 1: return 1;
case 2: return 2;
case 3: return 3;
}

But what you want is this:

switch (foo) {
    case 1: return 1;
    case 2: return 2;
    case 3: return 3;
}

Then you would apply the indent edit first and then apply an edit that just targeted indenting those case statements one level deeper.

And we can provide styleOptions that are passed into the style editors. You could very well have indentation info in that if necessary.

Just to reiterate: the upside here is that figuring out how to get the right code format output is on the end-user, and they can use any tool to do that.

Co-Authored-By: nzakas <nicholas@nczconsulting.com>
@nzakas
Copy link
Member Author

nzakas commented Jan 11, 2019

@kentcdodds thanks for the feedback. Yes, part of the goal here is to make things like eslint-plugin-prettier easier to create. Rather than needing to keep track of which rules to turn off, you'd just need to provide a config that includes a reference to the style editor that runs Prettier.

This is a great idea. Might I ask that in the event a style editor made caused a linting error a helpful error message is printed or some kinda of annotation on the error indicating that the style editor and linting rule conflict. This will make it much less frustrating for people who keep fixing the linting rule then run --fix and it's broken again.

That's a good suggestion, I'll add that in.

description: "A style editor just for ES5"
},
edits: [
// individual style edits
Copy link
Member

Choose a reason for hiding this comment

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

Are there use cases where the entries in the edits array would vary based on styleOptions? I'm wondering if it's more future-proof to make this similar to the rule API where we have meta and a function to return the edits array (whether that function is named create like in the rule API or still edits and we just check typeof styleEditor.edits === "function" before calling it or using its array value).

Everything I can think of could be done at the individual edit level, where options are already available in the current iteration of the RFC, but I'm curious whether you have ideas for future directions that might need the additional flexibility.

When ESLint is called with the `--fix` or `--fix-dry-run` flag, the style editor is applied to the source code after lint fixes are applied and before one final lint without fixes. The complete order of operations is as follows:

1. Preprocess source code using configured processors (if any)
1. Lint source code
Copy link
Member

Choose a reason for hiding this comment

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

As a performance optimization, it should be possible to run only those rules that specify the fixable property in this step.

An alternative design could have style editors implement an `edit()` method instead of an `edits` array. That design gives us less flexibility in a few ways:

1. With individual style edits, ESLint can determine the best way to apply one style after another. For instance, with a `text` style edit, ESLint knows that it's not necessary to parse the code before passing it into the style edit. Similarly, if there are two `text` style edits in a row, ESLint knows that the result of the first style edit doesn't need to be parsed yet and can be passed directly into the second `text` style edit. This flexibility of knowing what types of style edits need to be applied is important for performance optimiziation.
2. With a method, other style editors would have no choice but to apply all of another style editor before apply their own changes. With an array, it's possible for one style editor to pick and choose which edits from another style editor to include.
Copy link
Member

Choose a reason for hiding this comment

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

I imagine style editors' maintainers would want to add new edits in semver-minor releases, but this seems to indicate that the contents of the edits array within a style editor should be considered part of the editor's public API, making it immutable within a major version. This could be a case for the "Should style edits have IDs?" open question above so that style editors can compose edits from another style editor by ID rather than index.


This fixer simply normalizes line endings from Windows-style to Unix-style. It accesses the source code text through `context.sourceText` and returns a string. Each `text` fixer must return a string with the modifications to the source code.

The `context` object has the following properties:
Copy link
Member

Choose a reason for hiding this comment

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

Unless the user is using some 3rd party tool to parse and format the code, it seems like it would be fairly difficult to write a formatter. Do you see the main use-case here as using something like Prettier or Recast in the style enforcer?

@not-an-aardvark
Copy link
Member

Some of my comments have been scattered across multiple code review comments (thanks for bearing with me in this discussion), so I'll summarize my current point of view:

I think this RFC accurately identifies several problems with how ESLint is typically used nowadays (e.g. it's tedious to configure hundreds of stylistic rules, and people end up requesting a large number of options for core rules). However, it seems to me that almost all of the benefits of using a style editor could already be achieved by using a custom rule with autofixing. Rather than exporting a style editor that formats code a specific way, a plugin could instead export a rule that formats the user's entire file as an autofix.

To be clear, I realize it's not common for plugins to do this today. But given that it's already possible to create a rule that formats the entire file (and implementing a style editor would require a similar level of effort to implementing a rule), I'm not convinced it's worthwhile to add a style editor API. By encouraging plugin authors to implement rules that format an entire file, we could still choose to deprecate core stylistic rules if desirable, without adding unnecessary complexity to ESLint core.

@kaicataldo
Copy link
Member

@not-an-aardvark's comment above summarizes well my feelings about this proposal in its current state. I'm not convinced adding an additional API (and potentially confusion around what constitutes a "rule" vs a "style enforcer") is necessary when it can be achieved with the current rules API.

Do you mind expanding on why you think it's valuable to add a new API for this?

@vjeux
Copy link

vjeux commented Jan 19, 2019

One thing I don’t understand well is what’s the difference between this and a eslint rule that has a single autofix which runs prettier and outputs a big edit?

@nzakas nzakas added the Initial Commenting This RFC is in the initial feedback stage label Jan 29, 2019
@nzakas
Copy link
Member Author

nzakas commented Feb 11, 2019

Hey everyone, just a quick note to say I haven't forgotten about this! A couple more pressing issues came up that I had to address, and I don't have enough energy to keep up with all of it. I'll be back here once the other issues are resolved to answer outstanding questions and continue to iterate on this design. Thanks for your patience.

@nzakas
Copy link
Member Author

nzakas commented Jun 20, 2019

After giving it more thought, I'm withdrawing this RFC. I think this might make more sense as a standalone utility rather than part of ESLint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Initial Commenting This RFC is in the initial feedback stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants