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

max-len rule using indent_size property to set tabWidth #62

Closed
wants to merge 10 commits into from

Conversation

piranna
Copy link

@piranna piranna commented Jun 30, 2023

This PR both allow to pass multiple options to eslint, and also add a new max-len rule to use indent_size property as tabWidth value for eslint/max-len rule.

@piranna piranna requested a review from phanect as a code owner June 30, 2023 13:15
@phanect
Copy link
Owner

phanect commented Jun 30, 2023

Thanks, I'll check this PR tomorrow. (I'm in UTC+9 timezone)

@piranna
Copy link
Author

piranna commented Jun 30, 2023

Thanks, I'll check this PR tomorrow. (I'm in UTC+9 timezone)

Great, I'm at Spain (UTC +2).

Copy link
Owner

@phanect phanect left a comment

Choose a reason for hiding this comment

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

Can you write the application-level test?
(They are in tests/lib/rules/ directory)

I'm considering if "max-len": "off" should be included in plugin:editorconfig/noconflict because I think "editorconfig/max-len": "error" should not be included in plugin:editorconfig/all. (Unless you manually set code option, it does not work.)

lib/rules/max-len.js Outdated Show resolved Hide resolved
lib/base.js Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines +22 to +24
"max-len": [ "error", {
tabWidth: 2,
}],
Copy link
Owner

Choose a reason for hiding this comment

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

review:In My Opinion

Because this does nothing, how about adding code option?

Suggested change
"max-len": [ "error", {
tabWidth: 2,
}],
"max-len": [ "error", {
tabWidth: 2,
code: 80,
}],

Copy link
Author

Choose a reason for hiding this comment

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

If it's for testing reasons, we should use a value different of the default 80 columns.

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we do not have to add code option since it is set by default as you said.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe we do not have to add code option since it is set by default as you said.

tabWidth is also set by default to 4 columns, so we could remove it too.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, you can remove tabWidth option too. Or keeping it to explicitly define tabWidth is also OK if you prefer.

test-packages/success/js/.eslintrc.js Show resolved Hide resolved
test-packages/success/ts/.eslintrc.js Show resolved Hide resolved
main.js Outdated Show resolved Hide resolved
lib/rules/max-len.js Outdated Show resolved Hide resolved
description: "Enforce a maximum line length",
getESLintOption({ indent_size, max_line_length }) {
return {
enabled: max_line_length !== "off",
Copy link
Owner

Choose a reason for hiding this comment

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

This may cause an unexpected error when e.g. max_line_length === "foo".
How about enabled: !isNaN(max_line_length) && 0 < max_line_length?

Copy link
Author

Choose a reason for hiding this comment

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

Not sure if EditorConfig would validate the value, I don't think so, but it would not hurt doing it ourselves, I'll do it. For invalid values, eslint max-len set default 80 columns.

Copy link
Owner

@phanect phanect Jul 1, 2023

Choose a reason for hiding this comment

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

Not sure if EditorConfig would validate the value, I don't think so, but it would not hurt doing it ourselves, I'll do it.

Maybe we need to test it. I'm reviewing this on the smartphone now, so I will test it on the PC tomorrow.

For invalid values, eslint max-len set default 80 columns.

OK, that sounds intuitive.


However, there is another problem in enable: max_line_length !== "off".
When the user don't set max_line_length in .editorconfig, i.e. max_line_length === undefined, I think editorconfig/max-len should be disabled. However, the plugin reports errors if the line exceeds 80 characters. I don't think that's intuitive and consistent with other rules.

So I hope the following spec:

  • if max_line_length is number, editorconfig/max-len is enabled and this plugin checks if the line exceeds specified number of characters
  • if max_line_length is "off", editorconfig/max-len is disabled
  • if max_line_length is invalid value such as "foo",
    • editorconfig/max-len is enabled and this plugin checks if the line exceeds 80 characters, OR
    • editorconfig/max-len is disabled, OR
    • editorconfig/max-len reports a lint error to notify the user that invalid value set in max_line_length (This may be the best option if it can be quickly implemented.)
  • if max_line_length is NOT set in .editorconfig, editorconfig/max-len is disabled

How do you think about it?

@phanect
Copy link
Owner

phanect commented Jul 1, 2023

@phanect TODO Test if editorconfig/max-len works when linting TypeScript.
It might not work because there is no corresponding rule in @typescript-eslint.

@piranna
Copy link
Author

piranna commented Jul 1, 2023

@phanect TODO Test if editorconfig/max-len works when linting TypeScript. It might not work because there is no corresponding rule in @typescript-eslint.

It would use eslint max-len rule instead.

README.md Show resolved Hide resolved
description: "Enforce a maximum line length",
getESLintOption({ indent_size, max_line_length }) {
return {
enabled: max_line_length !== "off",
Copy link
Owner

@phanect phanect Jul 1, 2023

Choose a reason for hiding this comment

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

Not sure if EditorConfig would validate the value, I don't think so, but it would not hurt doing it ourselves, I'll do it.

Maybe we need to test it. I'm reviewing this on the smartphone now, so I will test it on the PC tomorrow.

For invalid values, eslint max-len set default 80 columns.

OK, that sounds intuitive.


However, there is another problem in enable: max_line_length !== "off".
When the user don't set max_line_length in .editorconfig, i.e. max_line_length === undefined, I think editorconfig/max-len should be disabled. However, the plugin reports errors if the line exceeds 80 characters. I don't think that's intuitive and consistent with other rules.

So I hope the following spec:

  • if max_line_length is number, editorconfig/max-len is enabled and this plugin checks if the line exceeds specified number of characters
  • if max_line_length is "off", editorconfig/max-len is disabled
  • if max_line_length is invalid value such as "foo",
    • editorconfig/max-len is enabled and this plugin checks if the line exceeds 80 characters, OR
    • editorconfig/max-len is disabled, OR
    • editorconfig/max-len reports a lint error to notify the user that invalid value set in max_line_length (This may be the best option if it can be quickly implemented.)
  • if max_line_length is NOT set in .editorconfig, editorconfig/max-len is disabled

How do you think about it?

@phanect
Copy link
Owner

phanect commented Jul 1, 2023

@phanect TODO Test if editorconfig/max-len works when linting TypeScript. It might not work because there is no corresponding rule in @typescript-eslint.

It would use eslint max-len rule instead.

OK, thanks for testing that.

@piranna
Copy link
Author

piranna commented Jul 1, 2023

When the user don't set max_line_length in .editorconfig, i.e. max_line_length === undefined, I think editorconfig/max-len should be disabled.

I don't think so, they are different things, one user can have max_line_length disabled, but still willing to have the rule enabled with the default 80 columns value, both by using the all rules, or by willing to use the tabWidth. If somebody wants to explicitly disable it, don't add the rule, or set max_line_length to off.

if max_line_length is invalid value such as "foo",
editorconfig/max-len is enabled and this plugin checks if the line exceeds 80 characters, OR
editorconfig/max-len is disabled, OR
editorconfig/max-len reports a lint error to notify the user that invalid value set in max_line_length (This may be the best option if it can be quickly implemented.)

eslint follows first option, I've implemented third one that's safest by throwing an exception, but not sure if we should remove validations and left eslint to do its job.

if max_line_length is NOT set in .editorconfig, editorconfig/max-len is disabled

I still thinks that if the rule is enabled but max_line_length is not set, it should use default values

@phanect
Copy link
Owner

phanect commented Jul 2, 2023

Sorry for the late reply. I fall asleep while I was reviewing yesterday 😅

When the user don't set max_line_length in .editorconfig, i.e. max_line_length === undefined, I think editorconfig/max-len should be disabled.

I don't think so, they are different things

Thanks for your idea.

The reason why I want to disable editorconfig/max-len when the user don't set max_line_length in .editorconfig is the consistency with other rules.
When other EditorConfig properties (e.g. trim_trailing_whitespace) are not set, the corresponding rule (e.g. editorconfig/no-trailing-spaces) is disabled. If editorconfig/max-len behaves differently, it should confuse the users.

one user can have max_line_length disabled, but still willing to have the rule enabled with the default 80 columns value, both by using the all rules, or by willing to use the tabWidth.

If max_line_length is not set but editorconfig/max-len is enabled (it warns exceeding 80 characters), the EditorConfig and ESLint's behaviors are not consistent either.
In this case, I think users should just set max_line_length = 80 in .editorconfig if they want to enable editorconfig/max-len.

If there is a usecase which requires to enable editorconfig/max-len when max_line_length is not set, I'm open to keep this behavior, but in that case, we need to carefully design the spec not to let users be confused.
We shouldn't add it to plugin:editorconfig/all for users who don't want to check max length of the lines. We also have to consider if we should add "max-len": "off" to plugin:editorconfig/noconflict. I guess there are many more things to consider.

if max_line_length is invalid value such as "foo",
editorconfig/max-len is enabled and this plugin checks if the line exceeds 80 characters, OR
editorconfig/max-len is disabled, OR
editorconfig/max-len reports a lint error to notify the user that invalid value set in max_line_length (This may be the best option if it can be quickly implemented.)

eslint follows first option, I've implemented third one that's safest by throwing an exception, but not sure if we should remove validations and left eslint to do its job.

Thanks, third option looks the best to me! I will test it.

@piranna
Copy link
Author

piranna commented Jul 2, 2023

The reason why I want to disable editorconfig/max-len when the user don't set max_line_length in .editorconfig is the consistency with other rules.
When other EditorConfig properties (e.g. trim_trailing_whitespace) are not set, the corresponding rule (e.g. editorconfig/no-trailing-spaces) is disabled. If editorconfig/max-len behaves differently, it should confuse the users.

I have read in detail the documentation of the other rules, both in this project and at eslint, and now I get your point about homogeneity, but at the same time, I think the approach is incorrect: if somebody came from using eslint in the past (like me :-) ) or is using the plugin as part of a shared config, he would spec that everything works by default. We are disabling the original rules and replacing them with our own ones, so without a .editorconfig file or without the proper editorconfig definitions, behaviour should be the same as with the original rules in that case. That means, rules should be working with the original eslint rules default values if their equivalent editorconfig ones are not provided (they are undefined), the same as what happens with the original eslint rules if you enable them but don't provide their config values. If not, what we are currently doing is to force everybody to have an .editorconfig file with all its definitions set, and silently fully disabling the rules if not. Based on the least-surprising-factor, I definitely find it a bug from user experience perspective...

If max_line_length is not set but editorconfig/max-len is enabled (it warns exceeding 80 characters), the EditorConfig and ESLint's behaviors are not consistent either.

Yes, they are: both max_line_length not set, or set to unset, means to use the editor values, and since we are using editorconfig to config the eslint rules, the "editor" is eslint, and "editor values" are the rules default values.

If there is a usecase which requires to enable editorconfig/max-len when max_line_length is not set, I'm open to keep this behavior, but in that case, we need to carefully design the spec not to let users be confused.

For example, if eslint-plugin-editorconfig is included as part of a shared config, and people (probably unknowingly) don't have a .editorconfig file. In that case, behaviour of all the rules should be the same as the original ones they are replacing :-)

We shouldn't add it to plugin:editorconfig/all for users who don't want to check max length of the lines. We also have to consider if we should add "max-len": "off" to plugin:editorconfig/noconflict. I guess there are many more things to consider.

Fair point, and considering that max-len is not enabled with eslint:recommended by default, we shouldn't either. In fact, none of the original rules are enabled by the eslint:recommended preset. So yes, we must it as part of a all preset (obviously :-) ), but we shouldn't add them for a (new) recommended one, to stick to the behaviour of the original eslint rules as much as possible.

Thanks, third option looks the best to me! I will test it.

I would have prefer to do nothing and left eslint do it at its own, but must to admit that using default value if setting a different value of off or a number is error prone and looks like an eslint bug to me... so better check and detect it earlier.

@phanect
Copy link
Owner

phanect commented Jul 2, 2023

I understand your perspective.

In general, this plugin is designed to force users to have .editorconfig. If the user add this plugin directly to their project (not via sharable config), I think we can ensure that .editorconfig exists because the users installed this plugin to use with EditorConfig.

However, when this plugin is called from sharable configs, we cannot guarantee that all users have (or want to have) .editorconfig in their project as you said.
Therefore, you think default value is required, right?

To solve this problem, I have considered to support the default options like following to allow sharable config creators to set default value:

{
  // ...
  "rules": {
    "editorconfig/eol-last": [ "error", { "default": "always" }],
    "editorconfig/indent": [ "error", { "default": 2 }],
    "editorconfig/linebreak-style": [ "error", { "default": "unix" }],
    "editorconfig/max-len": [ "error", { "default": { "tabWidth": 4, "code": 80 }}],
  },
  "plugins": [ "editorconfig" ]
}

If default is not set in .eslintrc or sharable configs, the rule is disabled when the corresponding EditorConfig is not set.
When the users use this plugin directly (without sharable configs), i.e. when the users can ensure that there is .editorconfig, they don't have to set default values in .eslintrc and .editorconfig property which they don't want to apply.

However, since we can't implememt the default value support immediately, I think editorconfig/max-len should be disabled when max_line_length is not set at the moment to keep the consistency. If you make editorconfig/max-len enabled by default, other rules should be enabled by default too, or the users are confused. (But I don't want to enable all the rules by default as explained in next paragraph)

I guess you focus on the consistency to follow the default behavior of original rules in ESLint. On the other hand, I focus on consistency to follow the .editorconfig and EditorConfig behavior. This plugin is "An ESLint plugin to enforce EditorConfig rules" as written in the description of this repository, so I want to make it consistent with EditorConfig behavior rather than ESLint's original rules.

Yes, they are: both max_line_length not set, or set to unset, means to use the editor values, and since we are using editorconfig to config the eslint rules, the "editor" is eslint, and "editor values" are the rules default values.

Logically true, but I don't think it is intuitive. In that case, EditorConfig follows the real editor (e.g. VSCode)'s default config, and this plugin follows ESLint default. They can be different.
In that logic, we need to follow the real editor's config to lint, while it is probably not possible.

In addition, I think EditorConfig does nothing when .editorconfig does not exist. I don't think it follows real editor's default.
For example, when insert_final_newline is not set or there is no .editorconfig, both EditorConfig and real editor do not insert or remove line, while EditorConfig insert or remove when insert_final_newline is set.

@piranna
Copy link
Author

piranna commented Jul 2, 2023

Therefore, you think default value is required, right?

No, I think it's just enough set it as undefined, and left eslint to apply its default values itself.

I guess you focus on the consistency to follow the default behavior of original rules in ESLint.

Yes, that's it.

On the other hand, I focus on consistency to follow the .editorconfig and EditorConfig behavior.

I was starting to suspect that...

I want to make it consistent with EditorConfig behavior

Then, you are giving me the reason: editorconfig behaviour is that if the definition is not provided or is unset, it takes the value provided by the editor that's interpreting them. You can think that's the user editor, but in fact, being an ESLint plugin, they are the ESLint rules the ones that are interpreting the editorconfig definitions, and the ones that provides the base behaviour (in this case, since the rule is enabled, imposing an 80 columns limit).

Logically true, but I don't think it is intuitive. In that case, EditorConfig follows the real editor (e.g. VSCode)'s default config, and this plugin follows ESLint default. They can be different.
In that logic, we need to follow the real editor's config to lint, while it is probably not possible.

Problem is that you are assuming the behaviour of a specific "real" editor, in that case VSCode, but people can be using other ones. VSCode don't have native support for editorconfig, you need to add an extension for that (there are several of them, in fact), so the one applying the default values is the extension, not VSCode editor, meanwhile other editor have native support for it. Here is similar, if you enable the rule, defaults are provided by the rule. What I would not find intuitive as an user is that a rule with multiple optional settings gets fully disabled and don't apply any of the settings just because one of them is not provided by the user, while in other environments (both using original eslint rules, or code editors using editorconfig) they are working totally fine.

In addition, I think EditorConfig does nothing when .editorconfig does not exist. I don't think it follows real editor's default.
For example, when insert_final_newline is not set or there is no .editorconfig, both EditorConfig and real editor do not insert or remove line, while EditorConfig insert or remove when insert_final_newline is set.

https://editorconfig.org/:

"Generally, if a property is not specified, the editor settings will be used, i.e. EditorConfig takes no effect on that part. For any property, a value of unset is to remove the effect of that property, even if it has been set before. For example, add indent_size = unset to undefine indent_size property (and use editor default)."

Check mate 😎

@phanect
Copy link
Owner

phanect commented Jul 2, 2023

Therefore, you think default value is required, right?

No, I think it's just enough set it as undefined, and left eslint to apply its default values itself.

OK, so adding default options to the rules does not work in your usecase? Can you elaborate why it doesn't work?

Applying ESLint value is not enough because it forces users to add max_line_length = unset when they don't want to limit wax line length.

I want to make it consistent with EditorConfig behavior

but in fact, being an ESLint plugin, they are the ESLint rules the ones that are interpreting the editorconfig definitions, and the ones that provides the base behaviour (in this case, since the rule is enabled, imposing an 80 columns limit).

Since I don't think it is intuitive to treat ESLint or this plugin as if they are an editor, I'm afraid I don't agree with this logic.

Problem is that you are assuming the behaviour of a specific "real" editor, in that case VSCode,

I couldn't get your point. Can you elaborate why the difference between VSCode (needs extension) and others (native EditorConfig support) matters?

What I would not find intuitive as an user is that a rule with multiple optional settings gets fully disabled and don't apply any of the settings just because one of them is not provided by the user, while in other environments (both using original eslint rules, or code editors using editorconfig) they are working totally fine.

Can't it be solved by adding default options to the rules of this plugin?
If the developer who create the sharable config add default options, you don't need to configure by yourself.

For now, I think it should be considered by design. The developer who created the sharable config expected you to have .editorconfig in your project.
This design can be improved, but it should not by degrading .editorconfig users' experience.

In addition, I think EditorConfig does nothing when .editorconfig does not exist. I don't think it follows real editor's default.

https://editorconfig.org/:

"Generally, if a property is not specified, the editor settings will be used, i.e. EditorConfig takes no effect on that part. ..."

OK, I understand it follows real editor's default.
However, it also says

i.e. EditorConfig takes no effect on that part.

so both your idea and my idea seems true.

Since it is probably impossible to follow real editor settings, I think this plugin can take no effect when the property is not specified.

Check mate 😎

Please refrain from treating the discussion like a battle. That makes the discussion unconstructive. Please try to be constructive.

It is 10pm in Japan, so I probably reply tomorrow night after this post.

@piranna
Copy link
Author

piranna commented Jul 2, 2023

OK, so adding default options to the rules does not work in your usecase? Can you elaborate why it doesn't work?

Because it's over complicated, it doesn't makes sense. KISS principle.

Applying ESLint value is not enough because it forces users to add max_line_length = unset when they don't want to limit wax line length.

That's not like that, according to the spec, setting max_line_length = unset unsets any previously assigned value, and it's the same as not defining max_line_length at all. If you consider that doing it that way force them to add max_line_length = unset, it's because you insist in that not setting it would fully disable the rule instead of using the default value. Using it would make things easier for everybody, since it's how the spec is intended for.

I couldn't get your point. Can you elaborate why the difference between VSCode (needs extension) and others (native EditorConfig support) matters?

In that each editor or extension can have and provide their own defaults, and that's why editorconfig is used for, but it doesn't enforce any standard default values or behaviour, but instead uses whatever one is provided by the editor, the extension, or any editorconfig interpreter, and that default values can be different from one to other, for example new lines are different on Windows or Linux or old MacOS Classic, and not defining end_of_line at all or defining it to unset would allow to use that new lines as default values, without disabling any other checks. Or you can set to use tabs, but some editors have 4 or 8 columns by default, and all of them are fine, but not having defined tab_width means that indent_style = tab should be disabled too.

Can't it be solved by adding default options to the rules of this plugin?
If the developer who create the sharable config add default options, you don't need to configure by yourself.

No, because:

  1. I'm the creator of the shareable config, and that's useless extra work
  2. I want to use the eslint defaults whatever they are, and not provide my own ones, also if eslint ones changes in the future

For now, I think it should be considered by design. The developer who created the sharable config expected you to have .editorconfig in your project.
This design can be improved, but it should not by degrading .editorconfig users' experience.

Me, as the developer that have created the shareable config, I want to work it properly for all the users, including the ones without an .editorconfig file, using in that case the eslint defaults as if they were using the original eslint rules. That's what as one of the shareable config users I would expect to be the least surprising.

OK, I understand it follows real editor's default.

Yes, being that a real editor, or the defaults provided by any editorconfig interpreter, in this case eslint.

However, it also says

i.e. EditorConfig takes no effect on that part.

so both your idea and my idea seems true.

What that means if that if a property is not specified, editorconfig behaviour is transitive, using the editor settings, no disabling the rule. If setting max_line_length = 80 has a different behaviour than not setting it at all, although eslint max-len default code value is also 80 columns, and by disabling the rule would provoque it because all the other settings of the rule would not be checked, then the plugin is not being transitive, and goes against the editorconfig spec.

Since it is probably impossible to follow real editor settings, I think this plugin can take no effect when the property is not specified.

And it's not possible to follow any real editor settings, because we don't know what's the one the user is using, but it also doesn't matter. Since we are running it with eslint, eslint defaults are the only ones that matter here, and the ones that must be used.

@phanect
Copy link
Owner

phanect commented Jul 3, 2023

OK, so adding default options to the rules does not work in your usecase? Can you elaborate why it doesn't work?

Because it's over complicated, it doesn't makes sense. KISS principle.

It is good to keep your implementation simple from the developers' perspective, but on the other hand, I think it makes the plugin more complicated from the users' perspective in this case.
I think keeping the user experience simple is more important than keeping the code simple when they don't consist with.

Applying ESLint value is not enough because it forces users to add max_line_length = unset when they don't want to limit wax line length.

That's not like that, according to the spec, setting max_line_length = unset unsets any previously assigned value, and it's the same as not defining max_line_length at all.

I also confirmed that setting max_line_length = unset and not defining max_line_length make the same behavior from the spec.
However, in your current implementation, max_line_length = unset disables editorconfig/max-len, but not defining max_line_length enables the rule with the default 80-character limit. Their behaviors are different. (Correct me if I misunderstand your implementation)
I think both behaviors should be consistent. (I prefer that both disable the rules of this plugin.)

Can't it be solved by adding default options to the rules of this plugin?
If the developer who create the sharable config add default options, you don't need to configure by yourself.

No, because:

  1. I'm the creator of the shareable config, and that's useless extra work
  2. I want to use the eslint defaults whatever they are, and not provide my own ones, also if eslint ones changes in the future

OK, regarding 2, how about supporting "eslint" as a value of default to enforce default configs of original rules on ESLint?

{
  // ...
  "rules": {
    // ...
    "editorconfig/max-len": [ "error", { "default": "eslint" }],
  },
  "plugins": [ "editorconfig" ]
}

I know this does not solve 1, but if this plugin enforces the default config of original rules, users who want to disable the rules of this plugin by removing the EditorConfig property need extra work to add (prop name) = unset instead.

So can you compromise by "default": "eslint" options support?

OK, I understand it follows real editor's default.

Yes, being that a real editor, or the defaults provided by any editorconfig interpreter, in this case eslint.

However, it also says

i.e. EditorConfig takes no effect on that part.

so both your idea and my idea seems true.

What that means if that if a property is not specified, editorconfig behaviour is transitive, using the editor settings, no disabling the rule. If setting max_line_length = 80 has a different behaviour than not setting it at all, although eslint max-len default code value is also 80 columns, and by disabling the rule would provoque it because all the other settings of the rule would not be checked, then the plugin is not being transitive, and goes against the editorconfig spec.

Sorry, I couldn't understand what you mean well.
What does "transitive" mean in this context?
What are "all the other settings of the rule"? max-len's other options like comments and ignorePattern?

If the property is not specified in .editorconfig, EditorConfig uses the default config of the editor (i.e. real editors like IntelliJ IDEA or plugins like VSCode's EditorConfig extension. In your idea, ESLint is also considered an editor), right?

I understand this behavior can be considered "disabling rules" because the official says

i.e. EditorConfig takes no effect on that part.

Therefore, I think it does not violate the EditorConfig spec by disabling the rules of this plugin when the corresponding EditorConfig property is not set, even if we treat ESLint as an Editor.

Since it is probably impossible to follow real editor settings, I think this plugin can take no effect when the property is not specified.

And it's not possible to follow any real editor settings, because we don't know what's the one the user is using, but it also doesn't matter. Since we are running it with eslint, eslint defaults are the only ones that matter here, and the ones that must be used.

As I said, I'm afraid I disagree with treating ESLint as an editor since it is unintuitive. Therefore, it is not necessary to apply ESLint defaults.
From the developers' perspective, it is logically possible to treat ESLint as if it is an editor, but from the users' perspective, it is definitely not an editor.

I also suspect that the spec really expects ESLint as an Editor because it is too different from normal (real) editors. (Normal editors have interactive CUI or GUI, while ESLint only has non-interactive CUI and API)
This eslint-plugin-editorconfig also behaves differently from normal Plugins like VSCode's extension. (Normal plugins automatically fix code on save to follow the config, but it is optional in eslint-plugin-editorconfig. Instead, eslint-plugin-editorconfig can show underlines on errors or raise an error on CI.)


Other thoughts:

  • I may have to learn more about your use cases so that I can suggest alternative solutions.
  • A new rule to check if .editorconfig exists may be useful to avoid forgetting to add .editorconfig, while I'm not sure if I can implement it.

Unfortunately, I'm busy tonight and my response may slow-paced. Thanks for understanding.

@piranna
Copy link
Author

piranna commented Jul 3, 2023

I think keeping the user experience simple is more important than keeping the code simple when they don't consist with.

Then, you confirm me adding a default option is counter-intuitive and make users life worse.

However, in your current implementation, max_line_length = unset disables editorconfig/max-len, but not defining max_line_length enables the rule with the default 80-character limit. Their behaviors are different. (Correct me if I misunderstand your implementation)
I think both behaviors should be consistent. (I prefer that both disable the rules of this plugin.)

Both cases uses the default 80 columns, that's the intended behaviour. The only option that disable the rule is max_line_length = off, that's how eslint max-len disables it. OTOH, "off" is not a valid value for max_line_length, so maybe it should be removed, preventing the rule to be disabled at all except if it's not done when configuring it.

OK, regarding 2, how about supporting "eslint" as a value of default to enforce default configs of original rules on ESLint?

Is default an option of eslint max-len rule? No, so it will surprise users, so it's not a solution for some rules that pretend to be a 1:1 replace of the original ones.

I know this does not solve 1, but if this plugin enforces the default config of original rules, users who want to disable the rules of this plugin by removing the EditorConfig property need extra work to add (prop name) = unset instead.

This is not based on the spec, as I confirmed yesterday, setting a property as unset has the same effect as not having it defined never at all, so we can't have a different behaviour, it's undefined at all effects.

Sorry, I couldn't understand what you mean well.
What does "transitive" mean in this context?

Transparent, that it doesn't affect the behaviour, like it doesn't exists at all.

What are "all the other settings of the rule"? max-len's other options like comments and ignorePattern?

Yes, the ones provided as options when defining the rule.

If the property is not specified in .editorconfig, EditorConfig uses the default config of the editor (i.e. real editors like IntelliJ IDEA or plugins like VSCode's EditorConfig extension. In your idea, ESLint is also considered an editor), right?

Yes, and the same for prettier with https://github.com/josephfrazier/editorconfig-to-prettier, or textlint with https://github.com/arrowrowe/textlint-rule-editorconfig.

Therefore, I think it does not violate the EditorConfig spec by disabling the rules of this plugin when the corresponding EditorConfig property is not set, even if we treat ESLint as an Editor.

It does, because we have enabled the rule in eslint config, so if the .editorconfig file doesn't have defined one of the properties, it needs to use the eslint default value for that property in the rule, not disabling the rule.

As I said, I'm afraid I disagree with treating ESLint as an editor since it is unintuitive. Therefore, it is not necessary to apply ESLint defaults.
From the developers' perspective, it is logically possible to treat ESLint as if it is an editor, but from the users' perspective, it is definitely not an editor.

Agree on that, maybe it's not "so much" intuitive because it's not a "real" editor, but definitely it's an editorconfig interpreter, and since we can't know what are the actual default values of the real editor that's being used by the user, our best bet is to use the actual eslint default values. In the end, editorconfig is intended to unify and makes homogeneous that values not only between different users editors, but also all the tools that they are making use of, so they doesn't need to define them in several places and have them sync'ed.

I also suspect that the spec really expects ESLint as an Editor because it is too different from normal (real) editors. (Normal editors have interactive CUI or GUI, while ESLint only has non-interactive CUI and API)

Yes, that's the point, from editorconfig spec, ESLint is another (non-interactive) editor.

This eslint-plugin-editorconfig also behaves differently from normal Plugins like VSCode's extension. (Normal plugins automatically fix code on save to follow the config, but it is optional in eslint-plugin-editorconfig. Instead, eslint-plugin-editorconfig can show underlines on errors or raise an error on CI.)

VSCode plugins does the change on saving only if configured so, but yes, I get your point. In the same way, you can config a eslint VSCode plugin to do the fixes on save, too.

  • A new rule to check if .editorconfig exists may be useful to avoid forgetting to add .editorconfig, while I'm not sure if I can implement it.

Interesting, I like that. Yes, this kind of project wide instead of file specific rules are difficult to implement for eslint, but definitely something to give it a try. I would not put it as an error by default in the all or recommended rules sets, since if the file doesn't exists, it's like the definitions are undefined, but as a warning to remind to do it would be nice.

Unfortunately, I'm busy tonight and my response may slow-paced. Thanks for understanding.

Ok, no issues, will be busy this week too.

@phanect
Copy link
Owner

phanect commented Jul 15, 2023

Sorry for the delayed reply.

OK, there are the following points:

  1. Does the EditorConfig spec disallows eslint-plugin-editorconfig to disable its rules when the property is not specified? in .editorconfig?
    a. Is ESLint an Editor according to the spec?
    b. If ESLint is an Editor, what should this plugin do when a property is not set in .editorconfig? Disable the corresponding rule or keep it enabled with ESLint/@typescript-eslint's default config?
  2. Which is intuitive, disabling the corresponding rule or keeping it enabled with ESLint/@typescript-eslint's default config?

1. Does the EditorConfig spec disallows eslint-plugin-editorconfig to disable its rules when the property is not specified?

1-a. Is ESLint an Editor according to the spec?

The Editor defined in EditorConfig spec primarily expects real editors. ESLint is much different from real editors, and it is not clarified in the spec if ESLint should be treated as an Editor.
Therefore, it is not required to treat ESLint as an Editor defined in the EditorConfig spec, while it can be considered an Editor if we wish.
I want to choose not to treat ESLint as an Editor.

1-b. If ESLint is an Editor, what should this plugin do when a property is not set in .editorconfig?

Even if we treat ESLint as an Editor, the spec does not clarify what should the Editor (i.e. ESLint) do when a property is not set in .editorconfig.
The official website says:

Generally, if a property is not specified, the editor settings will be used, i.e. EditorConfig takes no effect on that part. For any property, a value of "unset" is to remove the effect of that property, even if it has been set before.

and this helps us to understand the intent of the EditorConfig author(s).
According to this, "the editor settings will be used" and "EditorConfig takes no effect on that part".
In real editor, they are the same meanings. However, they are different behavior on ESLint since it is not a real editor. It is not clarified which behavior we should choose.

Therefore, we can choose using ESLint default or making no effect.
I want to choose making no effect.

Conclusion of 1

It is not clarified:

  • if ESLint is an Editor
  • if this plugin should use ESLint default or make no effect when EditorConfig property is not set
    in the EditorConfig spec.

Therefore, we can choose which we prefer.
I want to choose not to treat ESLint as an Editor, and even if I had to treat ESLint as an Editor, I want to choose making no effect when the EditorConfig property is unset.

2. Which is intuitive, disabling the corresponding rule or keeping it enabled with ESLint/@typescript-eslint's default config?

I noticed it is different by the user. I understand some people like you want this plugin to follow ESLint's default when the property is not set. However, the others like me want this plugin to do nothing in such cases.

Because this depends on the preferences, I think there is no conclusion.
To support various values, I think it is the best idea to allow customization.
That's why I think supporting "eslint" as a value of default may be the best option.

{
  // ...
  "rules": {
    // ...
    "editorconfig/max-len": [ "error", { "default": "eslint" }],
  },
  "plugins": [ "editorconfig" ]
}

Maybe we also need something like plugin:editorconfig/default-if-not-set ruleset for easier configuration. (Maybe we need a better preset name, BTW)

The ruleset looks like this:

module.exports = { 
  configs: { 
    "default-if-not-set": { 
      rules: {
         "eol-last": "off", 
         indent: "off", 
         "linebreak-style": "off", 
         "no-trailing-spaces": "off", 
         "unicode-bom": "off",
         "max-len": "off",
         "@typescript-eslint/eol-last": "off", 
         "@typescript-eslint/indent": "off", 
         "@typescript-eslint/linebreak-style": "off", 
         "@typescript-eslint/no-trailing-spaces": "off", 
         "@typescript-eslint/unicode-bom": "off",

        "editorconfig/charset": [ "error", { "default": "eslint" }],
        "editorconfig/no-trailing-spaces": [ "error", { "default": "eslint" }],
        "editorconfig/eol-last": [ "error", { "default": "eslint" }],
        "editorconfig/indent": [ "error", { "default": "eslint" }],
        "editorconfig/linebreak-style": [ "error", { "default": "eslint" }],
        "editorconfig/max-len": [ "error", { "default": "eslint" }],
      },
    },
  },
};

We might find out which is more intuitive through a survey, but I think it would be difficult to collect so many users' answers.


OK, regarding 2, how about supporting "eslint" as a value of default to enforce default configs of original rules on ESLint?

Is default an option of eslint max-len rule?

Let me clarify. default can receive following values:

  • "ignore": (default) do not lint by the rule if max_line_length is not set in .editorconfig.
  • "eslint": follow the ESLint's max-len default if max_line_length is not set in .editorconfig.
  • { tabWidth?: number, code: number }: set tabWidth and code if max_line_length is not set in .editorconfig. Other options can be set to 3rd elements of the array.

However, in your current implementation, max_line_length = unset disables editorconfig/max-len, but not defining max_line_length enables the rule with the default 80-character limit. Their behaviors are different.

Both cases uses the default 80 columns, that's the intended behaviour. The only option that disable the rule is max_line_length = off, that's how eslint max-len disables it. OTOH, "off" is not a valid value for max_line_length, so maybe it should be removed, preventing the rule to be disabled at all except if it's not done when configuring it.

OK, I understand your design.
If we only support this design, you cannot disable the rule through .editorconfig. That sounds inconvenient because you can no longer use plugin:editorconfig/all ruleset if you want to disable one of the rules, and it surprises existing users because it is a radical breaking change.

OK, regarding 2, how about supporting "eslint" as a value of default to enforce default configs of original rules on ESLint?

Is default an option of eslint max-len rule? No, so it will surprise users, so it's not a solution for some rules that pretend to be a 1:1 replace of the original ones.

Can you elaborate on why it surprises users? I don't think it is surprising.

Sorry, I couldn't understand what you mean well.
What does "transitive" mean in this context?

Transparent, that it doesn't affect the behaviour, like it doesn't exists at all.

Thanks, so it looks like we have a different understanding of EditorConfig's "transparency". I think the current behavior of this plugin is "transparent" because I understand EditorConfig's default behavior is not to do anything, but I understand using ESLint default can be considered "transparent" too.

If the property is not specified in .editorconfig, EditorConfig uses the default config of the editor (i.e. real editors like IntelliJ IDEA or plugins like VSCode's EditorConfig extension. In your idea, ESLint is also considered an editor), right?

Yes, and the same for prettier with https://github.com/josephfrazier/editorconfig-to-prettier, or textlint with https://github.com/arrowrowe/textlint-rule-editorconfig.

OK, thanks for confirmation.

IIRC prettier enforces prettier's default rules when the users don't configure prettier rules. Therefore, it makes sense to use prettier's default settings when the EditorConfig properties are not set. It is consistent to prettier's policy to create their "opinionated" linter.
I respect textlint-rule-editorconfig's author's decision, but I don't like that behavior because I think it is too opinionated to enforce the default rules.
Anyway, eslint-plugin-editorconfig has a different policy from them.

Therefore, I think it does not violate the EditorConfig spec by disabling the rules of this plugin when the corresponding EditorConfig property is not set, even if we treat ESLint as an Editor.

It does, because we have enabled the rule in eslint config, so if the .editorconfig file doesn't have defined one of the properties, it needs to use the eslint default value for that property in the rule, not disabling the rule.

Can you elaborate why we must ignore "i.e. EditorConfig takes no effect on that part"?

As I said, I'm afraid I disagree with treating ESLint as an editor since it is unintuitive. Therefore, it is not necessary to apply ESLint defaults.
From the developers' perspective, it is logically possible to treat ESLint as if it is an editor, but from the users' perspective, it is definitely not an editor.

Agree on that, maybe it's not "so much" intuitive because it's not a "real" editor, but definitely it's an editorconfig interpreter, ... our best bet is to use the actual eslint default values.

Is ESLint an EditorConfig interpreter? I think eslint-plugin-editorconfig is an EditorConfig interpreter.
Even if ESLint is an EditorConfig interpreter, it does not mean that ESLint is an Editor defined in EdotorConfig spec.

I also suspect that the spec really expects ESLint as an Editor because it is too different from normal (real) editors. (Normal editors have interactive CUI or GUI, while ESLint only has non-interactive CUI and API)

Yes, that's the point, from editorconfig spec, ESLint is another (non-interactive) editor.

I also suspect that the spec expects a non-interactive editor as an Editor.

This eslint-plugin-editorconfig also behaves differently from normal Plugins like VSCode's extension. (Normal plugins automatically fix code on save to follow the config, but it is optional in eslint-plugin-editorconfig. Instead, eslint-plugin-editorconfig can show underlines on errors or raise an error on CI.)

VSCode plugins does the change on saving only if configured so, but yes, I get your point. In the same way, you can config a eslint VSCode plugin to do the fixes on save, too.

Yes, so eslint-plugin-editorconfig does nothing until you configure VSCode's ESLint plugin (which is something not defined in the spec in this case).
I think this is much different behavior from normal Plugins defined in the spec.

@piranna
Copy link
Author

piranna commented Jul 15, 2023

The Editor defined in EditorConfig spec primarily expects real editors. ESLint is much different from real editors, and it is not clarified in the spec if ESLint should be treated as an Editor.

An editor is any software that can edit text files, so ESLint can be considered one. Maybe not a "real" user oriented interactive editor, but definitely, ESList in fact IS a text editor. In any case, probably due to that artificial distinction between "real" editors and other editorconfig compliant tools, in other parts of the spec they talks about editorconfig "interpreters" instead.

Therefore, it is not required to treat ESLint as an Editor defined in the EditorConfig spec, while it can be considered an Editor if we wish. I want to choose not to treat ESLint as an Editor.

Definitely I find it a bad decision, but it's your decision. If so, you should clarify it in the README.md file to advice the users that would be expecting it, because as this discussion has shown, the decision about considering eslint-plugin-editorconfig an editor or not affect on its actual behaviour. Intuitively, if I find a package named eslint-plugin-editorconfig, what I expect from its name is that it will make ESLint to add support for .editorconfig file and follow and enforce its rules the same way it would do editorconfig plugin on VSCode, or any other editorconfig plugin for any other "real" editor, in case I receive contributions from somebody that doesn't make use of an editorconfig enabled "real" editor. At least for me, not considering ESLint an editor makes this plugin totally useless.

Therefore, we can choose using ESLint default or making no effect.
I want to choose making no effect.

Again, bad decision. The spec is clear in this aspect: if a editorconfig is not provided, or its final resolution is "unset", that means user doesn't care or actual settings for that value and we must use editor default values, in this case ESLint default values.

I want to choose not to treat ESLint as an Editor, and even if I had to treat ESLint as an Editor, I want to choose making no effect when the EditorConfig property is unset.

Spec is clear:

  1. ESLint is an editor
  2. if a rule is enabled, but an editorconfig property is unset, we must use ESLint default value

It's your package, and they are your decisions, but they make this module not compliant with editorconfig spec, and that should be put on the README.md file to notify users about that fact.

However, the others like me want this plugin to do nothing in such cases.

Has anybody contacted you requesting this behaviour? Because I can only think this spec faulty behaviour has only got unnoticed until now because just by chance editorconfig defaults are the same of ESLint config ones and they are not interested on using some values outside the standard ones...

Let me clarify. default can receive following values:

  • "ignore": (default) do not lint by the rule if max_line_length is not set in .editorconfig.
  • "eslint": follow the ESLint's max-len default if max_line_length is not set in .editorconfig.

As I've said, this is totally cumbersome and useless, the spec is clear. Anyway, I'll follow you the game: if we take that path, default value must be eslint, since it's what the spec dictates and the user lest surprising option, since some rules can have several config options and nobody spec them to be fully disabled just because one of their optional values with some defaults gets unset.

  • { tabWidth?: number, code: number }: set tabWidth and code if max_line_length is not set in .editorconfig. Other options can be set to 3rd elements of the array.

This is ridiculous, and enforces my point of view: why define some specific default values, when you can already define them in the options field of the rule itself, as you can do with ESLint built-in rules? This is just defining the same thing in two different places, making it more confusing to users, and defeat one of the core principles of using editorconfig in the first place: DRY.

Both cases uses the default 80 columns, that's the intended behaviour. The only option that disable the rule is max_line_length = off, that's how eslint max-len disables it. OTOH, "off" is not a valid value for max_line_length, so maybe it should be removed, preventing the rule to be disabled at all except if it's not done when configuring it.

OK, I understand your design. If we only support this design, you cannot disable the rule through .editorconfig. That sounds inconvenient because you can no longer use plugin:editorconfig/all ruleset if you want to disable one of the rules, and it surprises existing users because it is a radical breaking change.

My wrong, I got swapped the terms here: max_line_length = off is how editorconfig disabled the rule, ESLint doesn't have any way to disable a rule except on rules config. What I mean here, is that it would make sense to prevent usage of max_line_length = off or ignore it since there's no way to do it on ESLint from the ESLint side, meanwhile it would make sense to endorse it and disable the rule from the editorconfig side. Maybe the editorconfig behaviour would be the correct here, somebody that sets max_line_length = off doesn't want to have a line length limit at all.

OK, regarding 2, how about supporting "eslint" as a value of default to enforce default configs of original rules on ESLint?

Is default an option of eslint max-len rule? No, so it will surprise users, so it's not a solution for some rules that pretend to be a 1:1 replace of the original ones.

Can you elaborate on why it surprises users? I don't think it is surprising.

It's surprising for people that just enable eslint-plugin-editorconfig and expect that it will just replace ESLint built-in rules with some editorconfig tainted ones, without need of any extra configuration or tune-up, also without the need to add an .editorconfig file. With your proposal, doing just this way the behaviour will change, and they need to set both a default: 'eslint' value in the rule options and add a .editorconfig file to keep their current behaviour, not fun.

IIRC prettier enforces prettier's default rules when the users don't configure prettier rules. Therefore, it makes sense to use prettier's default settings when the EditorConfig properties are not set. It is consistent to prettier's policy to create their "opinionated" linter.
I respect textlint-rule-editorconfig's author's decision, but I don't like that behavior because I think it is too opinionated to enforce the default rules.
Anyway, eslint-plugin-editorconfig has a different policy from them.

The thing is that I think using the default prettier or textlint values is the less opinionated option, since they are using what their systems provides to them, and are transparent on this way. By doing nothing and disabling rules by not being fully configured (but otherwise totally correct), I think the opinionated one here is the behaviour of eslint-plugin-editorconfig module.

Therefore, I think it does not violate the EditorConfig spec by disabling the rules of this plugin when the corresponding EditorConfig property is not set, even if we treat ESLint as an Editor.

It does, because we have enabled the rule in eslint config, so if the .editorconfig file doesn't have defined one of the properties, it needs to use the eslint default value for that property in the rule, not disabling the rule.

Can you elaborate why we must ignore "i.e. EditorConfig takes no effect on that part"?

Because if editorconfig would not be here, ESLint rules default values apply here, that what "i.e. EditorConfig takes no effect on that part" means here: both having unset properties, or not making use of editorconfig at all, produce exactly the same results. If not having editorconfig uses the ESLint default values, and having it but with unset properties produce a different result (in this case, disable a rule, for example), then it's not "takes no effect" at all.

Is ESLint an EditorConfig interpreter? I think eslint-plugin-editorconfig is an EditorConfig interpreter.

It is, with the help of eslint-plugin-editorconfig. In the same way, you would argument that VSCode is not an editorconfig interpreter, because you need the vscode-editorconfig plugin, but you are all the time refering to VSCode as an editorconfig "real" editor although it doesn't have native support for editorconfig at all...

Even if ESLint is an EditorConfig interpreter, it does not mean that ESLint is an Editor defined in EdotorConfig spec.

It is, since it's capable of format and write the provided files. It's not an user oriented, interactive editor, but definitely it's a text editor. In that way, I would think of it as sed command, that's in fact non-user oriented, non-interactive text editor directed by some user provided rules.

Or if you want something more explicit, you have eclint: a linter that only purpose is to enforce editorconfig rules, definitely it's a linter and not an user-oriented interactive editor, but is it an editorconfig editor? Uneasy philosofical question, I know... Now replace it's internal engine for ESLint one and apply the rules of eslint-plugin-editorconfig. What's the difference? None at all: if one can be considered an editorconfig editor, the other one having a similar behaviour and results can be considered too, and if not, then eslint-plugin-editorconfig module should change its name because it would not be following the editorconfig spec anymore, and it would be an "editorconfig-like plugin for ESLint" instead.

I also suspect that the spec expects a non-interactive editor as an Editor.

Yes, that's the most simple interpretation of the spec, at least we agree on that.

This eslint-plugin-editorconfig also behaves differently from normal Plugins like VSCode's extension. (Normal plugins automatically fix code on save to follow the config, but it is optional in eslint-plugin-editorconfig. Instead, eslint-plugin-editorconfig can show underlines on errors or raise an error on CI.)

VSCode plugins does the change on saving only if configured so, but yes, I get your point. In the same way, you can config a eslint VSCode plugin to do the fixes on save, too.

Yes, so eslint-plugin-editorconfig does nothing until you configure VSCode's ESLint plugin (which is something not defined in the spec in this case). I think this is much different behavior from normal Plugins defined in the spec.

Not so much, as I told you, you also needs to enable the VSCode-editorconfig plugin to do the fixing on save, or the same for prettier. The fact of being done the fixings automatically on saving or manually doesn't get any difference at all, the important thing is that when they get executed, no matter when that happens, ALL of them have the same behaviour and produce the same results, no matter what editor they are using, being VSCode, prettier, ESLint, GEdit, textlint, nano, vim, sed... or also handwritten pen and paper: all must enforse and have the same behaviour regarding the editorconfig definitions, and if any of them is undefined, use each editor particular default values instead.

@phanect
Copy link
Owner

phanect commented Jul 17, 2023

The Editor defined in EditorConfig spec primarily expects real editors.

An editor is any software that can edit text files, so ESLint can be considered one.

Literally, it is possible to call ESLint a (text) editor.
However, when you call some software "(text) editor", does it always include non-interactive editors like sed? I guess you don't. At least I don't.
"(text) editor" might sometimes mean non-interactive editors, but not frequently. That's why I think it is not clear if ESLint should be considered an Editor defined in the spec.

I want to choose not to treat ESLint as an Editor.

If so, you should clarify it in the [README.md](http://readme.md/) file

True. Since eslint-plugin-editorconfig behaves differently from the Editor or Plugin defined in the spec, it is required to add an explanation.
I will add an explanation about this behavior in README or the rule docs when I have time.

Intuitively, if I find a package named eslint-plugin-editorconfig, what I expect from its name is that it will make ESLint to add support for .editorconfig file and follow and enforce its rules the same way it would do editorconfig plugin on VSCode, or any other editorconfig plugin for any other "real" editor, in case I receive contributions from somebody that doesn't make use of an editorconfig enabled "real" editor.

I think the current version of this plugin enforces the rules in the same way as the EditorConfig extension of VSCode, but we have different understandings of "the same way".

Therefore, we can choose using ESLint default or making no effect.
I want to choose making no effect.

The spec is clear in this aspect: if a editorconfig is not provided, or its final resolution is "unset", that means user doesn't care or actual settings for that value and we must use editor default values, in this case ESLint default values.

I couldn't find such text which says "unset" means "use the editor's (or plugin's?) default values" and it does not mean "do nothing" from the spec.
Can you quote the text from the spec?

However, the others like me want this plugin to do nothing in such cases.

Has anybody contacted you requesting this behaviour?

No, but no one requested me to use ESLint rules' default other than you either.

  • { tabWidth?: number, code: number }: set tabWidth and code if max_line_length is not set in .editorconfig. Other options can be set to 3rd elements of the array.

This is ridiculous, and enforces my point of view: why define some specific default values, when you can already define them in the options field of the rule itself, as you can do with ESLint built-in rules? This is just defining the same thing in two different places, making it more confusing to users, and defeat one of the core principles of using editorconfig in the first place: DRY.

Yes, I may need some improvement for this design. I will consider a better one.

OK, regarding 2, how about supporting "eslint" as a value of default
Is default an option of eslint max-len rule? No, so it will surprise users,

Can you elaborate on why it surprises users? I don't think it is surprising.

It's surprising for people that just enable eslint-plugin-editorconfig and expect that it will just replace ESLint built-in rules with some editorconfig tainted ones, without need of any extra configuration or tune-up, also without the need to add an .editorconfig file. With your proposal, doing just this way the behaviour will change, and they need to set both a default: 'eslint' value in the rule options and add a .editorconfig file to keep their current behaviour, not fun.

OK, so this is surprising for people like you because they don't think the EditorConfig Plugins (such as the VSCode extension) are doing nothing when the EditorConfig property is not set, but they think the Plugins are following the Editor or Plugin's default.
It depends on how each person understands EditorConfig's default behavior, so I think there is no design that is not surprising for everyone.

IIRC prettier enforces prettier's default rules when the users don't configure prettier rules. Therefore, it makes sense to use prettier's default settings when the EditorConfig properties are not set. It is consistent to prettier's policy to create their "opinionated" linter.
I respect textlint-rule-editorconfig's author's decision, but I don't like that behavior because I think it is too opinionated to enforce the default rules.
Anyway, eslint-plugin-editorconfig has a different policy from them.

The thing is that I think using the default prettier or textlint values is the less opinionated option, since they are using what their systems provides to them, and are transparent on this way. By doing nothing and disabling rules by not being fully configured (but otherwise totally correct), I think the opinionated one here is the behaviour of eslint-plugin-editorconfig module.

Sorry, I started an unconstructive discussion. It is probably impossible to judge which is more opinionated. It depends on the person.

Can you elaborate why we must ignore "i.e. EditorConfig takes no effect on that part"?

Because if editorconfig would not be here, ESLint rules default values apply here, that what "i.e. EditorConfig takes no effect on that part" means here: both having unset properties, or not making use of editorconfig at all, produce exactly the same results. If not having editorconfig uses the ESLint default values, and having it but with unset properties produce a different result (in this case, disable a rule, for example), then it's not "takes no effect" at all.

So your understanding of "EditorConfig takes no effect on that part" is different from mine.

Is ESLint an EditorConfig interpreter? I think eslint-plugin-editorconfig is an EditorConfig interpreter.

It is, with the help of eslint-plugin-editorconfig. In the same way, you would argument that VSCode is not an editorconfig interpreter, because you need the vscode-editorconfig plugin, but you are all the time refering to VSCode as an editorconfig "real" editor although it doesn't have native support for editorconfig at all...

Here's my understanding:
VSCode is an Editor and not an EditorConfig interpreter.
VSCode's EditorConfig extension is a Plugin and an EditorConfig interpreter.

But I noticed this is not related to the entire discussion so much. Let's stop discussing this.

Even if ESLint is an EditorConfig interpreter, it does not mean that ESLint is an Editor defined in EdotorConfig spec.

It is, since it's capable of format and write the provided files. It's not an user oriented, interactive editor, but definitely it's a text editor. In that way, I would think of it as sed command, that's in fact non-user oriented, non-interactive text editor directed by some user provided rules.

Or if you want something more explicit, you have eclint: a linter that only purpose is to enforce editorconfig rules, definitely it's a linter and not an user-oriented interactive editor, but is it an editorconfig editor? Uneasy philosofical question, I know... Now replace it's internal engine for ESLint one and apply the rules of eslint-plugin-editorconfig. What's the difference? None at all: if one can be considered an editorconfig editor, the other one having a similar behaviour and results can be considered too, and if not, then eslint-plugin-editorconfig module should change its name because it would not be following the editorconfig spec anymore, and it would be an "editorconfig-like plugin for ESLint" instead.

is it an editorconfig editor? Uneasy philosofical question, I know...

That's the point. It is difficult to judge if ESLint is an EditorConfig Editor. That's why I think it depends on your and my understanding.

What's the difference? None at all: if one can be considered an editorconfig editor, the other one having a similar behaviour and results can be considered too,

I couldn't understand the logic. Even if there is no difference between eclint and ESLint + eslint-plugin-editorconfig currently, it does not mean that you must not add a different behavior from eclint to this plugin. Correct me if I misunderstand what you mean.

if not, then eslint-plugin-editorconfig module should change its name because it would not be following the editorconfig spec anymore, and it would be an "editorconfig-like plugin for ESLint" instead.

I couldn't understand the logic either.
Since I think the EditorConfig spec does not cover ESLint and eslint-plugin-editorconfig's behavior when the properties are not set, I think eslint-plugin-editorconfig follows the EditorConfig spec.

Yes, so eslint-plugin-editorconfig does nothing until you configure VSCode's ESLint plugin (which is something not defined in the spec in this case). I think this is much different behavior from normal Plugins defined in the spec.

Not so much, as I told you,

It is not possible to conclude "how much" different eslint-plugin-editorconfig and the normal EditorConfig Plugins are, so let's stop discussing this too.


I think it is probably impossible to conclude this discussion because it highly depends on our values and understanding of the spec.
If you cannot accept default: "eslint" style configuration, I have to recommend you fork this plugin, unfortunately.
I'm open to any other ideas to optionally allow users to configure to use the default values of the original ESLint rules, but you want this plugin to use the ESLint's default values by default, don't you?

@piranna
Copy link
Author

piranna commented Jul 17, 2023

Literally, it is possible to call ESLint a (text) editor.
However, when you call some software "(text) editor", does it always include non-interactive editors like sed? I guess you don't. At least I don't.
"(text) editor" might sometimes mean non-interactive editors, but not frequently. That's why I think it is not clear if ESLint should be considered an Editor defined in the spec.

I can understand that call ESLint an editor you need some imagination, and that sed is not the first one when you think about "editor", but definitely it is.

I will add an explanation about this behavior in README or the rule docs when I have time.

Ok.

I couldn't find such text which says "unset" means "use the editor's (or plugin's?) default values" and it does not mean "do nothing" from the spec.
Can you quote the text from the spec?

https://spec.editorconfig.org/: "For any pair, a value of unset removes the effect of that pair, even if it has been set before. For example, add indent_size = unset to undefine the indent_size pair (and use editor defaults)."

No, but no one requested me to use ESLint rules' default other than you either.

Fair enough. I can only think the reason is because the default values are the same, so that got unnoticed, and I'm the only one that wanted to ensure they were used.

Yes, I may need some improvement for this design. I will consider a better one.

👍🏻

OK, so this is surprising for people like you because they don't think the EditorConfig Plugins (such as the VSCode extension) are doing nothing when the EditorConfig property is not set, but they think the Plugins are following the Editor or Plugin's default.
It depends on how each person understands EditorConfig's default behavior, so I think there is no design that is not surprising for everyone.

It's as simple as to check what the VSCode plugin does if there's no .editorconfig file or there's a definition missing. I'm totally sure that they use the editor (VSCode) defaults, and the same for this plugin, that should use the parent plugin defaults instead.

It is probably impossible to judge which is more opinionated. It depends on the person.

Not so much, it's just a matter of interference and do the less work as possible. In this case, use the default values, and left the "opinionated" responsability to ESLint default values.

VSCode is an Editor and not an EditorConfig interpreter.
VSCode's EditorConfig extension is a Plugin and an EditorConfig interpreter.

Agree. I would also say, VSCode editorconfig extension makes VSCode an editorconfig interpreter, because it adds it that functionality, but without that plugin, definitely VSCode is not an editorconfig interpreter by itself.

That's the point. It is difficult to judge if ESLint is an EditorConfig Editor. That's why I think it depends on your and my understanding.

In my opinion, with the eslint-plugin-editorconfig, yes, it is, because the plugin provides that functionality.

it does not mean that you must not add a different behavior from eclint to this plugin

Yes, it does, because the editorconfig spec is intended to provide the same behavior on all the editorconfig interpreters implementing it. If any of them doesn't provides the same behaviour, also if that means using default values specific (and different) to each one of them, then it will not provide the expected behaviour according to the spec, that's the point of having one on the first place.

Since I think the EditorConfig spec does not cover ESLint and eslint-plugin-editorconfig's behavior when the properties are not set, I think eslint-plugin-editorconfig follows the EditorConfig spec.

But the thing is that it doesn't! Once the rules are enabled, they must follow the editorconfig behaviour, also by using default values when one editorconfig definition is not provided. If not, what's the purpose of enabling the rules?

you want this plugin to use the ESLint's default values by default, don't you?

Yes, because it's what the spec dictates. Anything besides that, it's a bug or custom behaviour, but definitely not according to the editorconfig spec.

@phanect
Copy link
Owner

phanect commented Jul 17, 2023

you want this plugin to use the ESLint's default values by default, don't you?

Yes,

OK, let me close this PR.

Unfortunately, it is not possible to find the sole right answer to the preferences or the issues with much room for interpretation.
I tried to accept your idea by adding it as an option, but if you believe the implementation based on your own preference and interpretation is the sole right one and do not admit the others, I can do nothing for you.

@phanect phanect closed this Jul 17, 2023
@piranna
Copy link
Author

piranna commented Jul 17, 2023

if you believe the implementation based on your own preference and interpretation is the sole right one and do not admit the others, I can do nothing for you

It's not that I believe my own preference and interpretation is the sole right, specs are not open to interpretation or that would lead to undefined behaviour, I have show you with arguments that the implementation is wrong, and you also admitted it and stills want to continue it that way, then there's no more discussion here: this plugin is not compliant with both editorconfig spec and ESLint conventions, so it's incompatible with any other software that follows them. I have tried to do my best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants