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

Should only touch modified lines? #106

Open
matthijskooijman opened this issue May 18, 2018 · 8 comments · May be fixed by #195
Open

Should only touch modified lines? #106

matthijskooijman opened this issue May 18, 2018 · 8 comments · May be fixed by #195
Labels

Comments

@matthijskooijman
Copy link

I just opened a file in a project that uses trim_trailing_whitespace=true. I made some changes to the file, and after saving it found that all lines had been stripped of trailing whitespace, not just the ones that I touched.

This is of course very annoying when you have files in version control, where you do not want to be faced with a lot of unrelated changes (of course files should be ok in the repo to begin with, but it's not an ideal world we live in).

It seems this is also the intention of the editorconfig spec. Their FAQ says:

Existing files are not reformatted. Only newly input lines are formatted in the format given in the .editorconfig files. If you are looking for a tool to check or/and reformat your existing files, check editorconfig-tools, eclint and editorconfig-cf (none of them work perfectly though).

Could something like this be implemented? Perhaps the current behaviour could be enabled by an option (or an explicit command that does a one-off run to fix all formatting)?

@xuhdev
Copy link
Member

xuhdev commented May 18, 2018

I somewhat agree with you, but it seems like all other plugins (AFAIK) implement as what this one is doing. In this case, should we correct the FAQ? For your case, I think it is better to add a new value, say newline, for trim_trailing_whitespace. Do you agree?

@xuhdev xuhdev added the Bug label May 18, 2018
@matthijskooijman
Copy link
Author

I'm not really sure if such a setting belongs in editorconfig, or should be plugin-specific. You could say that editorconfig contains the preferences of a project about how the files should look, while this is a preference of a user that indicates how (aggresive) to get to this ideal state. Also, this does not seem like a setting limited to the the trim_trailing_whitespace setting, but might be more of a global setting.

Looking at the rest of the settings, there are more settings where this distinction (fix-everything or only fix-changed-lines) could be relevant, though none of them seem as clear-cut:

  • indent_style: Lines that use tabs or spaces inconsistently with the tab settings should probably be subject to the same setting. Automatically fixing this might be tricky, especially when the inconsistent lines were made with different tab stop settings.
  • end_of_line: This should probably always be fix-everything, since otherwise you'll get files with inconsistent line endings. However, I believe files should preserve current line endings unless fix-everything is explicitly requested, making this setting mostly apply to new files.
  • insert_final_newline: This should probably be changed only when touching the last line, also to prevent unrelated changes.

Again, I'm not sure how much of this stuff belongs in editorconfig and how much is plugin-specific, but it might be useful if the editorconfig spec makes some recommendations about how to implement this. I would argue for a conservative fix-changed-lines approach by default, while also recommending editors to offer an explicit "fix-everything-now" command as well.

@davidfetter
Copy link

I've gotten around this via git tricks with telling its diff to ignore whitespace-only changes. Having something that didn't automagically require these tricks would be nice.

@lkraav
Copy link

lkraav commented Aug 27, 2018

I'm not sure if mixed-indent model makes sense, otherwise what's the purpose of .editorconfig? Mixed state gets specifically warning-flagged in my editors.

Full file .editorconfig-compliant indent conversion should be a separate commit first, after which everything can be rainbows and unicorns for follow-up edits.

@matthijskooijman
Copy link
Author

I'm not sure if mixed-indent model makes sense, otherwise what's the purpose of .editorconfig? Mixed state gets specifically warning-flagged in my editors.

For indents, this indeed does not make too much sense (ideal would be to match the indent to the surroundings if it does not match .editorconfig, but that's not really feasible).

However, for trailing whitespace, it can make sense to fix lines incrementally, when these lines are edited for other changes anyway (perhaps followed by a small cleanup-only commit after a while).

@lkraav
Copy link

lkraav commented Aug 27, 2018

However, for trailing whitespace

LOL, it's what happens when I try to look smart before the morning coffee 👍

Yep, I would agree, trailing whitespace can be handled incrementally.

Even though...

alias find-kill-trailing-whitespace='find . -path ./.git -prune -o \( -regextype posix-egrep -regex ".*\.(css|js|less|php|scss)" \) -exec sed -i "s/ *$//" {} \;'

still ftw, maybe? One-shot codebase cleanup (mod extension whitelist to suit your needs).

@oblitum
Copy link

oblitum commented Dec 21, 2018

I've hit this sometimes but I didn't expect to change plugin or standard, if a project provides a .editorconfig and its files are not following it, the project must fix that, that was my conclusion.

FlexW pushed a commit to FlexW/editorconfig-vim that referenced this issue Jul 27, 2022
@FlexW FlexW linked a pull request Jul 27, 2022 that will close this issue
FlexW pushed a commit to FlexW/editorconfig-vim that referenced this issue Jul 28, 2022
@mmrwoods
Copy link
Contributor

FWIW, this can be achieved using https://github.com/ntpeters/vim-better-whitespace, which supports stripping only modified lines (relies on external diff command to identify modified lines)...

" Disable editorconfig trim_trailing_whitespace, use better_whitespace
let g:EditorConfig_disable_rules = ['trim_trailing_whitespace']

" Automatically strip whitespace on save using better_whitespace plugin
let g:strip_whitespace_on_save = 1

" Only strip whitespace on modified lines and don't ask for confirmation
let g:strip_only_modified_lines = 1
let g:strip_whitespace_confirm = 0

" Use editorconfig hook to automatically enable/disable whitespace stripping
function! <SID>EditorConfigHook(config)
  try
    let l:trim_trailing_whitespace = a:config->get('trim_trailing_whitespace')
    if l:trim_trailing_whitespace == 'true'
      exec 'EnableStripWhitespaceOnSave'
    elseif l:trim_trailing_whitespace == 'false'
      exec 'DisableStripWhitespaceOnSave'
    endif
  catch
    echo 'EditorConfigHook Failed: ' . v:exception
  endtry
  return 0 " Return 0 to show no error happened
endfunction
call editorconfig#AddNewHook(function('<SID>EditorConfigHook'))

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

Successfully merging a pull request may close this issue.

6 participants