-
Notifications
You must be signed in to change notification settings - Fork 5
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
Conversation
Thanks, I'll check this PR tomorrow. (I'm in UTC+9 timezone) |
Great, I'm at Spain (UTC +2). |
There was a problem hiding this 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.)
"max-len": [ "error", { | ||
tabWidth: 2, | ||
}], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
description: "Enforce a maximum line length", | ||
getESLintOption({ indent_size, max_line_length }) { | ||
return { | ||
enabled: max_line_length !== "off", |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 TODO Test if |
It would use eslint |
description: "Enforce a maximum line length", | ||
getESLintOption({ indent_size, max_line_length }) { | ||
return { | ||
enabled: max_line_length !== "off", |
There was a problem hiding this comment.
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?
OK, thanks for testing that. |
I don't think so, they are different things, one user can have
I still thinks that if the rule is enabled but |
Sorry for the late reply. I fall asleep while I was reviewing yesterday 😅
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.
If If there is a usecase which requires to enable editorconfig/max-len when
Thanks, third option looks the best to me! I will test it. |
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
Yes, they are: both
For example, if
Fair point, and considering that
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 |
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. To solve this problem, I have considered to support the {
// ...
"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 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.
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 addition, I think EditorConfig does nothing when .editorconfig does not exist. I don't think it follows real editor's default. |
No, I think it's just enough set it as
Yes, that's it.
I was starting to suspect that...
Then, you are giving me the reason: editorconfig behaviour is that if the definition is not provided or is
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.
"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 😎 |
OK, so adding Applying ESLint value is not enough because it forces users to add
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.
I couldn't get your point. Can you elaborate why the difference between VSCode (needs extension) and others (native EditorConfig support) matters?
Can't it be solved by adding 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.
OK, I understand it follows real editor's default.
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.
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. |
Because it's over complicated, it doesn't makes sense. KISS principle.
That's not like that, according to the spec, setting
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
No, because:
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
Yes, being that a real editor, or the defaults provided by any editorconfig interpreter, in this case eslint.
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
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. |
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 also confirmed that setting
OK, regarding 2, how about supporting {
// ...
"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 So can you compromise by
Sorry, I couldn't understand what you mean well. 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
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.
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. 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) Other thoughts:
Unfortunately, I'm busy tonight and my response may slow-paced. Thanks for understanding. |
Then, you confirm me adding a
Both cases uses the default 80 columns, that's the intended behaviour. The only option that disable the rule is
Is
This is not based on the spec, as I confirmed yesterday, setting a property as
Transparent, that it doesn't affect the behaviour, like it doesn't exists at all.
Yes, the ones provided as options when defining the rule.
Yes, and the same for
It does, because we have enabled the rule in eslint config, so if the
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.
Yes, that's the point, from editorconfig spec, ESLint is another (non-interactive) editor.
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.
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
Ok, no issues, will be busy this week too. |
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?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. 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.
and this helps us to understand the intent of the EditorConfig author(s). Therefore, we can choose using ESLint default or making no effect. Conclusion of 1It is not clarified:
Therefore, we can choose which we prefer. 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. {
// ...
"rules": {
// ...
"editorconfig/max-len": [ "error", { "default": "eslint" }],
},
"plugins": [ "editorconfig" ]
} Maybe we also need something like The ruleset looks like this:
We might find out which is more intuitive through a survey, but I think it would be difficult to collect so many users' answers.
Let me clarify.
OK, I understand your design.
Can you elaborate on why it surprises users? I don't think it is surprising.
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.
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.
Can you elaborate why we must ignore "i.e. EditorConfig takes no effect on that part"?
Is ESLint an EditorConfig interpreter? I think eslint-plugin-editorconfig is an EditorConfig interpreter.
I also suspect that the spec expects a non-interactive editor as an Editor.
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). |
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.
Definitely I find it a bad decision, but it's your decision. If so, you should clarify it in the
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.
Spec is clear:
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
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...
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
This is ridiculous, and enforces my point of view: why define some specific default values, when you can already define them in the
My wrong, I got swapped the terms here:
It's surprising for people that just enable
The thing is that I think using the default
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.
It is, with the help of
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 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
Yes, that's the most simple interpretation of the spec, at least we agree on that.
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 |
Literally, it is possible to call ESLint a (text) editor.
True. Since eslint-plugin-editorconfig behaves differently from the Editor or Plugin defined in the spec, it is required to add an explanation.
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".
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.
No, but no one requested me to use ESLint rules' default other than you either.
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.
Sorry, I started an unconstructive discussion. It is probably impossible to judge which is more opinionated. It depends on the person.
So your understanding of "EditorConfig takes no effect on that part" is different from mine.
Here's my understanding: But I noticed this is not related to the entire discussion so much. Let's stop discussing this.
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.
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.
I couldn't understand the logic either.
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. |
I can understand that call ESLint an editor you need some imagination, and that
Ok.
https://spec.editorconfig.org/: "For any pair, a value of
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.
👍🏻
It's as simple as to check what the VSCode plugin does if there's no
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.
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.
In my opinion, with the
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.
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?
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. |
Co-authored-by: Jumpei Ogawa <phanect@users.noreply.github.com>
Co-authored-by: Jumpei Ogawa <phanect@users.noreply.github.com>
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. |
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. |
This PR both allow to pass multiple options to eslint, and also add a new
max-len
rule to useindent_size
property astabWidth
value foreslint/max-len
rule.