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

Add default editorconfig feature #645

Merged
merged 1 commit into from
Feb 27, 2023

Conversation

LEI
Copy link
Contributor

@LEI LEI commented Feb 4, 2023

This adds support for EditorConfig via ec4rs including domain-specific properties and should close #75 once the special cases are settled:

  • end_of_line = line_endings
    cr = Unix (Mac)
  • indent_size/tab_width = indent_width
  • indent_style = indent_type
  • max_line_length = column_width
  • quote_type = quote_style
    double = AutoPreferDouble
    single = AutoPreferSingle
    auto = StyLua default
  • call_parentheses
  • collapse_simple_statement
  • sort_requires

EditorConfig properties charset, insert_final_newline and trim_trailing_whitespace are not supported.

The .editorconfig file in the current directory is loaded if no .stylua.toml file was found: not sure if it's the desired behavior and how it should handle different directories. I used pub mod editorconfig in src/lib.rs in order to run tests on a real directory structure.

@LEI LEI marked this pull request as draft February 4, 2023 14:26
@codecov
Copy link

codecov bot commented Feb 4, 2023

Codecov Report

Base: 97.27% // Head: 97.27% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (93d8ec1) compared to base (06a761e).
Patch coverage: 96.41% of modified lines in pull request are covered.

❗ Current head 93d8ec1 differs from pull request most recent head b2a236d. Consider uploading reports for the commit b2a236d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #645      +/-   ##
==========================================
- Coverage   97.27%   97.27%   -0.01%     
==========================================
  Files          15       16       +1     
  Lines        5845     6124     +279     
==========================================
+ Hits         5686     5957     +271     
- Misses        159      167       +8     
Impacted Files Coverage Δ
src/lib.rs 97.29% <ø> (+0.45%) ⬆️
src/editorconfig.rs 96.41% <96.41%> (ø)
src/context.rs 98.16% <0.00%> (+0.91%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@LEI LEI marked this pull request as ready for review February 7, 2023 18:07
src/editorconfig.rs Outdated Show resolved Hide resolved
Copy link
Owner

@JohnnyMorganz JohnnyMorganz left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this!

A few things:

  • What happens when there doesn't exist any .editorconfig file at all? It looks like we "apply" the default properties from ec4rs, but are they the same as the stylua defaults? Is it possible to detect that no .editorconfig file was found, and just use Config::default()? Maybe there should be a test here to ensure the final config == Config::default()

  • Maybe we should search in parent directories for .editorconfig as well when --search-parent-directories is enabled

  • Looks like there are some unrelated changes, probably to make clippy happy. I guess its fine to leave them in this PR

README.md Outdated Show resolved Hide resolved
tests/test_editorconfig.rs Outdated Show resolved Hide resolved
src/cli/config.rs Outdated Show resolved Hide resolved
src/editorconfig.rs Outdated Show resolved Hide resolved
src/editorconfig.rs Outdated Show resolved Hide resolved
tests/inputs-editorconfig/.editorconfig Outdated Show resolved Hide resolved
@LEI
Copy link
Contributor Author

LEI commented Feb 9, 2023

  • The properties are now parsed only if they are found by the library.
    Maybe there is a cleaner way to check if each key is matching, with some kind of hash.
    I couldn't find how to use ::key() in match cases (not static).

  • The core library looks up parent directories itself (until root = true), as it is expected with EditorConfig.

The ignored test in tests/test_editorconfig.rs for patterns like [directory/file.lua] was failing because find_editorconfig needed the exact file name, this is now fixed.

Each file in a same directory could have a different configuration, though it is probably not a common use case.
Not sure how (multiple?) files provided in command-line arguments should be handled.

@LEI LEI force-pushed the feature-editorconfig branch 3 times, most recently from bbc02f1 to 4362162 Compare February 11, 2023 12:26
@LEI
Copy link
Contributor Author

LEI commented Feb 11, 2023

EditorConfig overrides are applied in src/cli/config.rs only if there is a standard input (- in args), based on stdin-filepath or the current directory.

Otherwise, the configuration overrides are loaded for each file in src/cli/main.rs, maybe the stdin check could be done around here for consistency.

@JohnnyMorganz
Copy link
Owner

JohnnyMorganz commented Feb 11, 2023

EditorConfig overrides are applied in src/cli/config.rs only if there is a standard input (- in args), based on stdin-filepath or the current directory.

Does this mean if I run stylua path/to/foo.lua it will still look for an editorconfig file? If not, is there a reason this functionality is removed?

Nevermind, I misunderstood. For non-stdin, you check individually for each file in main.rs.

Could you pull in the latest changes from main? I've resolved the clippy lints there so it can be removed from this PR

@JohnnyMorganz
Copy link
Owner

Otherwise, the configuration overrides are loaded for each file in src/cli/main.rs, maybe the stdin check could be done around here for consistency.

Yeah, I think this would be better, and then src/cli/config.rs can be left alone.

The properties are now parsed only if they are found by the library.
Maybe there is a cleaner way to check if each key is matching, with some kind of hash.
I couldn't find how to use ::key() in match cases (not static).

Is it necessary to loop through the properties? It might just be simpler if its something like

fn load(config: Config, properties: &Properties) -> Config {
    let mut new_config = config;
    
    // Handle standard keys
    if let Ok(end_of_line) = properties.get::<EndOfLine>() {
         // ...
    }
    if let Ok(indent_size) = properties.get::<IndentSize>() {
         // ...
    }
    // ...

   // Handle custom keys

Also, for the custom keys, is it worth creating a custom enum for them and impl PropertyKey + PropertyValue? If its not too necessary, feel free to leave it

Should no_call_parentheses and tab_width be parsed?

Sorry - I missed this. I don't think no_call_parentheses should be included (its deprecated). tab_width should be I think though, especially if indent_size points to UseTabWidth

I think we should also have a test with both an .editorconfig file and a stylua.toml file to ensure that latter is used.

Could you also update the CHANGELOG (add something into the Added section).

Sorry for the long review time, thank you once again for adding this!

@LEI
Copy link
Contributor Author

LEI commented Feb 12, 2023

Fixes:

  • Moved the logic to main, config::load_config now returns None if no stylua.toml was found.
  • Changed the properties matching to a if sequence and extracted the property_choice macro for enums.
  • tab_width is now handled (maybe properties.use_fallbacks() should be commented out).

TODO:

  • Improve stylua.toml + .editorconfig test
  • CRLF test always passes (attempt to keep EOL with .gitattributes)
  • Add support for sort_requires?

Copy link
Owner

@JohnnyMorganz JohnnyMorganz left a comment

Choose a reason for hiding this comment

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

Thanks! Just a few more comments which are hopefully a bit more minor.
Also, could we add a CLI option --no-editorconfig to ignore editorconfig completely like prettier does: https://prettier.io/docs/en/cli.html#--no-editorconfig

  • Add support for sort_requires?

Yeah, I guess we can do

src/cli/main.rs Outdated Show resolved Hide resolved
src/editorconfig.rs Outdated Show resolved Hide resolved
src/cli/main.rs Outdated Show resolved Hide resolved
src/editorconfig.rs Outdated Show resolved Hide resolved
@@ -7,6 +7,8 @@ use wasm_bindgen::prelude::*;

#[macro_use]
mod context;
#[cfg(feature = "editorconfig")]
pub mod editorconfig;
Copy link
Owner

Choose a reason for hiding this comment

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

Full public visibility to editorconfig is probably not ideal, since it shouldn't be an API consumers rely on. I don't think there is much we can do about it though, pub(crate) is probably too restrictive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, there are ways around it but it seems more involved.

I just pushed some cleanup, hopefully fixing the case where the input file would be left modified if test_stylua_toml failed, and removing the EOL test attempt (too tricky).

My last concern would be MaxLineLen::Off => config.with_column_width(0) since I did not test if 0 === off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

max_line_length = off was not working, fixed in the latest commit using usize::MAX instead of 0.

@LEI
Copy link
Contributor Author

LEI commented Feb 13, 2023

Reordered the functions in editorconfig.rs and added sort_requires = true, or should it be sort_requires_enabled?

@JohnnyMorganz
Copy link
Owner

Reordered the functions in editorconfig.rs and added sort_requires = true, or should it be sort_requires_enabled?

Probably best to use sort_requires_enabled for the sake of forwards compatibility in case future options are added to the sort requires code mod

@LEI
Copy link
Contributor Author

LEI commented Feb 14, 2023

Future options could work like this and it saves a few characters, unless I am missing something:

sort_requires = true
sort_requires_new_option = true

@JohnnyMorganz
Copy link
Owner

Future options could work like this and it saves a few characters, unless I am missing something:


sort_requires = true

sort_requires_new_option = true

Yeah you're right. Let's leave it as-is then

Copy link
Owner

@JohnnyMorganz JohnnyMorganz left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this longstanding issue!

I'm happy with the general implementation here. I don't know if you have anymore outstanding TODOs. If not, I'll probably merge by the end of this week and get a release out then

@LEI LEI force-pushed the feature-editorconfig branch from 61ed0cb to 93d8ec1 Compare February 15, 2023 07:58
LEI pushed a commit to LEI/StyLua that referenced this pull request Feb 15, 2023
@LEI LEI force-pushed the feature-editorconfig branch from 93d8ec1 to 3d450c4 Compare February 15, 2023 19:39
LEI pushed a commit to LEI/StyLua that referenced this pull request Feb 15, 2023
@LEI LEI force-pushed the feature-editorconfig branch from 3d450c4 to fff4c56 Compare February 15, 2023 20:00
LEI added a commit to LEI/StyLua that referenced this pull request Feb 17, 2023
@LEI LEI force-pushed the feature-editorconfig branch from fff4c56 to 8e9d5a4 Compare February 17, 2023 17:00
LEI added a commit to LEI/StyLua that referenced this pull request Feb 17, 2023
@LEI LEI force-pushed the feature-editorconfig branch from 8e9d5a4 to 24d4c3d Compare February 17, 2023 17:02
@LEI LEI force-pushed the feature-editorconfig branch from 24d4c3d to b2a236d Compare February 17, 2023 17:12
@LEI
Copy link
Contributor Author

LEI commented Feb 17, 2023

The test for max line length is now updated and commits are squashed.

Should be ready to go if you are okay with pub mod editorconfig.

Copy link
Owner

@JohnnyMorganz JohnnyMorganz left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you, and sorry for the delay!

@JohnnyMorganz JohnnyMorganz merged commit d6ed051 into JohnnyMorganz:main Feb 27, 2023
@LEI LEI deleted the feature-editorconfig branch February 27, 2023 17:24
MisanthropicBit added a commit to MisanthropicBit/winmove.nvim that referenced this pull request Sep 6, 2023
@mindbound
Copy link

mindbound commented Nov 19, 2023

What is the reason why insert_final_newline is not supported?

@JohnnyMorganz
Copy link
Owner

Stylua does not respect the option itself - it will always add a newline.

Although, it should parse the option and ignore it - an error is not useful if you are using the editor config for multiple languages

@mindbound
Copy link

That's extremely odd - in both my neovim setup and running from command line, stylua does the exact opposite: it always removes the final newline from the formatted file. That's why I was asking the previous question - i.e., hoping that there is an option to prevent this behaviour.

@JohnnyMorganz
Copy link
Owner

Huh, that's definitely weird. I just tried stylua from the command line right now and it added a new line
image

@mindbound
Copy link

I'm getting the exact opposite: after adding a newline after local x = 1, it gets removed on formatting. I'm using Arch, stylua installed from the official extra repository, version 0.19.1.

@JohnnyMorganz
Copy link
Owner

JohnnyMorganz commented Nov 19, 2023

Sorry, I would have no clue how to diagnose further as I do not have a setup such as yours. All our test cases have a final newline (+1 extra that is added by the snapshot testing library) so I would assume it is something to do with your setup.

Stupid question - but could it be whatever you are using to render your file is hiding the final newline? Note that stylua does trim multiple final newlines into only one newline, that may be hidden by your editor.

@mindbound
Copy link

mindbound commented Nov 19, 2023

That was my initial thought as well but apparently not, this is really what happens:

image

The stylua binary is is from the official Arch repo, so presumably without any weird third-party modifications.

@JohnnyMorganz
Copy link
Owner

That is really weird. Sorry I don't have a better answer for you, I'm curious what is causing this

@JohnnyMorganz
Copy link
Owner

Wait a second - isn't that output correct? Your initial test.lua file has two newlines, which we trim down to 1

wc -l test.lua would report 0 if there was no trailing newline, and you would see something like this the first line in cat:

image

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.

[Suggestion] Support .editorconfig
3 participants