Skip to content

Conversation

@lewis6991
Copy link
Member

@lewis6991 lewis6991 commented Aug 23, 2023

@lewis6991 lewis6991 requested a review from gpanders August 23, 2023 10:25
@lewis6991 lewis6991 marked this pull request as draft August 23, 2023 10:34
@lewis6991 lewis6991 added the needs:discussion issue needs attention from an expert, or PR proposes significant changes to architecture or API label Aug 23, 2023
@github-actions github-actions bot removed the request for review from gpanders August 23, 2023 10:36
@lewis6991

This comment was marked as resolved.

@ObserverOfTime

This comment was marked as resolved.

@gpanders
Copy link
Member

Supporting off spec entries might not be the best path forward. However, I found the warning very intrusive whilst trying to browse a project.

I also do not feel good about supporting off-spec values, particularly this one which is just a no-op (it's equivalent to not setting the property at all. Why??)

I don't think it should be the job of our editorconfig plugin to validate .editorconfig files, so an error Is probably a bit much.

Either:

  • warning, once only
  • no disruptive message, but send to a log file somewhere.

I think validating the .editorconfig values is valuable to catch typos/common errors (is it indent_style or indent_type? space or spaces?), but agreed that an error is probably too aggressive. One-time warning sounds good to me.

@lewis6991
Copy link
Member Author

I think validating the .editorconfig values is valuable to catch typos/common errors (is it indent_style or indent_type? space or spaces?),

Then ideally these diagnostics should be thrown when editing a .editorconfig file, not when browsing any file in the repo. Maybe this is another candidate for an in-process LSP(?), or something like our TS query linter. Anyway, that's out of scope here, I'll make this a warning once.

@gpanders
Copy link
Member

I made a PR to LuaLS to drop the off-spec value LuaLS/lua-language-server#2287.

@gpanders
Copy link
Member

Then ideally these diagnostics should be thrown when editing a .editorconfig file, not when browsing any file in the repo.

Interesting idea, I hadn't thought of that but seems reasonable.

@lewis6991 lewis6991 changed the title fix(editorconfig): support end_of_line = 'auto' fix(editorconfig): only warn once on errors Aug 23, 2023
@lewis6991
Copy link
Member Author

Done. Improved the typing a bit in typical fashion.

@lewis6991 lewis6991 marked this pull request as ready for review August 23, 2023 13:23
@lewis6991 lewis6991 merged commit dc45fb4 into neovim:master Aug 23, 2023
@lewis6991 lewis6991 deleted the fix/editrcofnig branch August 23, 2023 13:45
@zeertzjq zeertzjq added backport release-0.9 and removed needs:discussion issue needs attention from an expert, or PR proposes significant changes to architecture or API labels Aug 27, 2023
@zeertzjq zeertzjq added this to the 0.9.2 milestone Aug 27, 2023
@github-actions
Copy link
Contributor

Successfully created backport PR for release-0.9:

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