search

atom / whitespace

Atom whitespace package
MIT License
94 stars 61 forks source link

Add option to only trim whitespace on modified lines if project is a Git-repo #168

Closed sorawee closed 4 years ago

sorawee commented 6 years ago

Requirements

Description of the Change

This PR adds an option to only trim whitespace on modified lines if project is a Git-repo. It revives an old PR #57 which was closed because the original author (@johanlunds) did not finish it.

This PR differs from the original PR in two aspects:

  1. It uses JavaScript instead of CoffeeScript.
  2. It has tests.

Alternate Designs

In some editors, this feature applies to files that are not in a git repo too. However, it requires an API from the editor to track changes from an unmodified file. Since Atom did not provide this API, the original author restricts tracking to only files in a Git project (where we can track changes from an unmodified file via Git), which is the best we could do.

Benefits

While it is true that getting rid of trailing whitespaces is a good idea, when users work on a project that does not have a policy regarding whitespaces, modifying a file with Atom often results in a very big diff because of the trailing whitespace removal. Consequently, reviewing the diff becomes difficult because of the noisy change.

Some users avoid this problem by disabling the trailing whitespace removal entirely, but as I mentioned above, getting rid of trailing whitespaces is a good idea, so this is not ideal.

This PR combines the best of the two: it allows trailing space removal, but it doesn't create a large diff since trailing space removal only occurs on lines that are touched.

(Also, this feature has been requested since 2014, with at least 50 upvotes!)

Possible Drawbacks

I guess it can increase the time when removing trailing spaces on a big file with large diff since we need to ask git where the unmodified lines are. However, users can disable this feature easily to avoid the problem if this issue ever happens.

Applicable Issues

8 : Option to kill trailing whitespace only on lines I touch

sorawee commented 6 years ago

Can anyone tell what's going on with AppVeyor build? When I tested locally, I saw the output just like what Travis CI outputs (which is successful).

Arcanemagus commented 6 years ago

Is there a reason you went with the "VCS only" approach instead of the generic "only lines touched since the file was opened"?

IanVS commented 6 years ago

This would be a really awesome feature to have. The PR has been open for a while with no movement. What would it take to push across the finish line? @sorawee did you lose interest?

IanVS commented 6 years ago

Or, at this point would it be better to close this and take the approach suggested in https://github.com/atom/whitespace/issues/8#issuecomment-350858835?

sorawee commented 6 years ago

Feel free to close. I am not familiar with the non-VCS approach and I no longer use Atom.