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

Do not use off-spec value for end_of_line #2287

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

gpanders
Copy link
Contributor

The EditorConfig specification does not support "auto" as a value for "end_of_line". EditorConfig implementations which adhere to the specification (such as Neovim) will either ignore this value or, worse, show a warning or error to the user. Simply omitting end_of_line will use the default end of line value for the platform.

Git (and other source control tools) can handle translating end of line characters between Windows and Unix platforms. The end_of_line option is there only when you want to maintain a strict end of line character across platforms.

The [EditorConfig specification](https://spec.editorconfig.org/#supported-pairs) does not support "auto" as a value for "end_of_line". EditorConfig implementations which adhere to the specification (such as Neovim) will either ignore this value or, worse, show a warning or error to the user. Simply omitting `end_of_line` will use the default end of line value for the platform.

Git (and other source control tools) can handle translating end of line characters between Windows and Unix platforms. The `end_of_line` option is there _only_ when you want to maintain a strict end of line character *across* platforms.
@sumneko
Copy link
Collaborator

sumneko commented Aug 25, 2023

@CppCXY

@CppCXY
Copy link
Collaborator

CppCXY commented Aug 26, 2023

What I don't understand is that your editorconfig plugin only supports the specification, but why delete our configuration, does the configuration of luals affect the use of others?

@lewis6991
Copy link
Contributor

lewis6991 commented Aug 26, 2023

It means people who use other editors get error notifications when trying to browse this repository.

What do you expect auto to actually do? Does it have defined behaviour?

Correct me if I'm wrong, but my guess is that it does nothing and your editor (vscode?) is just silently ignoring the invalid value.

https://github.com/editorconfig/editorconfig-vscode/blob/main/src/transformations/SetEndOfLine.ts#L6

@CppCXY
Copy link
Collaborator

CppCXY commented Aug 26, 2023

in vscode, the editorconfig plugin doesn't warn me that auto is an error value.
and when formatting, editorconfig is actually read and parsed by LuaLs (EmmyLuaCodeStyle) itself, and we do not use any third-party libraries

@CppCXY
Copy link
Collaborator

CppCXY commented Aug 26, 2023

Actually, I didn't want to implement the 'auto' value at first, see CppCXY/EmmyLuaCodeStyle#20, (you can use translator)
we had discussions, and I eventually accepted the auto option, which at the time neovim did not support editorconfig

@lewis6991
Copy link
Contributor

in vscode, the editorconfig plugin doesn't warn me that auto is an error value.

Yes. I linked the code. Vscode silently ignores the invalid value.

and when formatting, editorconfig is actually read and parsed by LuaLs (EmmyLuaCodeStyle) itself, and we do not use any third-party libraries

It sounds like it would make more sense for your formatter to infer the behaviour of auto if end_of_line isn't found.

To me it makes little sense to be adding custom fields to .editorconfig. Especially in this case where it isn't needed.

@CppCXY
Copy link
Collaborator

CppCXY commented Aug 26, 2023

It sounds like it would make more sense for your formatter to infer the behaviour of auto if end_of_line isn't found.

To me it makes little sense to be adding custom fields to .editorconfig. Especially in this case where it isn't needed.

No, for a project, formatting the configuration is a value that the project needs to know explicitly in addition to the tool needs to read. and it is better to have a clear definition of the format displayed, rather than relying on default behavior of tools.

@lewis6991
Copy link
Contributor

No, for a project, formatting the configuration is a value that the project needs to know explicitly in addition to the tool needs to read. and it is better to have a clear definition of the format displayed, rather than relying on default behavior of tools.

Then this config should live in some other configuration file that isn't .editorconfig. Or rename .editorconfig to some other name since currently it isn't valid.

@CppCXY
Copy link
Collaborator

CppCXY commented Aug 26, 2023

Then this config should live in some other configuration file that isn't .editorconfig. Or rename .editorconfig to some other name since currently it isn't valid.

We are arguing about why we should remove the configuration of our project instead of discussing whether 'end_of_line' should have an 'auto' value or suggesting to rename the configuration file. Our consensus is that 'auto' is not a standard value and nvim also warns against using this value. We all believe it is reasonable, but why do we have to remove the configuration from our project?

@lewis6991
Copy link
Contributor

but why do we have to remove the configuration from our project?

My comment gave two suggestions that involved not removing the configuration.

The resolution I want here is that the .editorconfig file is valid. I don't mind how that is achieved whether that is removing or moving the configuration.

@CppCXY
Copy link
Collaborator

CppCXY commented Aug 26, 2023

but why do we have to remove the configuration from our project?

My comment gave two suggestions that involved not removing the configuration.

The resolution I want here is that the .editorconfig file is valid. I don't mind how that is achieved whether that is removing or moving the configuration.

There seems to be a problem here. Why does the .editorconfig file in the luals project have to comply with the standard? We all believe in pragmatism, and we think that the configuration options supported by editorconfig are not sufficient. In addition, my statement was just an explanation of why we chose to add this option. Sumneko himself may be more stubborn on how his project should be done, so it is ultimately his decision.

@lewis6991
Copy link
Contributor

We all believe in pragmatism

If this is true, then I would have thought you would like to remove friction for users of other editors other than vscode from contributing to the project, because as of now the .editorconfig file in this project is only compatible with vscode.

@CppCXY
Copy link
Collaborator

CppCXY commented Aug 26, 2023

We all believe in pragmatism

If this is true, then I would have thought you would like to remove friction for users of other editors other than vscode from contributing to the project, because as of now the .editorconfig file in this project is only compatible with vscode.

Until now, you have only explained why you needed to remove our configuration. All I wanted was a reason, not whether it should or should not be done. I believe this is a reasonable request, but the final decision should be made by @sumneko

@lewis6991
Copy link
Contributor

Sorry I wasn't clear about this. I implied this in my first comment on this thread:

It means people who use other editors get error notifications when trying to browse this repository.

@CppCXY
Copy link
Collaborator

CppCXY commented Aug 26, 2023

why not use 'unset' instead of remove the config?

@gpanders
Copy link
Contributor Author

why not use 'unset' instead of remove the config?

unset is also fine, since that is a valid value. But it has the same effect as just removing it.

We are arguing about why we should remove the configuration of our project instead of discussing whether 'end_of_line' should have an 'auto' value

Whether or not 'end_of_line' should have an “auto” value is a different discussion and out of scope here. That needs to be brought up to the EditorConfig project. But the entire point of having a specification is to have agreed upon values and semantics. If an individual project makes up new values that are not in the specification then that is going to cause problems for projects (like Neovim) that do follow the specification.

Other editors (like VS Code) may not show a warning or error for the invalid value (though it does simply ignore it, as @lewis6991 pointed out). We have taken the decision in Neovim to show a warning because it is useful to let users know if they’ve made a mistake in their .editorconfig file.

@sumneko
Copy link
Collaborator

sumneko commented Aug 29, 2023

In fact, I don't use the formatting feature at all now, and I haven't included formatting as a requirement for the PR, so this file hasn't been used. I can consider deleting the entire file. Formatting requires a lot of detailed configuration to fit the current code style, and I think it would be better to leave it to AI to do in the future.

@lewis6991
Copy link
Contributor

I think indent_style, indent_size, tab_width and insert_final_newline are still pretty useful to have since they are used to configure editors, even if there is a formatter that automatically runs during CI.

@lewis6991
Copy link
Contributor

Is there any chance in just merging this as is? I just got bugged again by it whilst working on #2346

@sumneko sumneko merged commit b74ee5b into LuaLS:master Oct 10, 2023
7 checks passed
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.

4 participants