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

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

Open
joshbetz opened this issue Feb 27, 2014 · 50 comments
Open

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

joshbetz opened this issue Feb 27, 2014 · 50 comments

Comments

@joshbetz
Copy link

@joshbetz joshbetz commented Feb 27, 2014

If I edit a file with lots of trailing whitespace and change one line, the diff is going to be hard to read if all the whitespace is stripped. I would still like to make sure I'm not introducing trailing whitespace and even strip it from lines that I've edited.

@torarnv
Copy link

@torarnv torarnv commented Feb 27, 2014

Yes please!

@torarnv
Copy link

@torarnv torarnv commented Feb 27, 2014

Same goes for the trailing new-line (I guess?). Should only be added if I touched one of the last lines so that the final line is included in the diff.

@ho0ber
Copy link

@ho0ber ho0ber commented Feb 27, 2014

I strongly agree that this would be an excellent option to have.

@watsonian
Copy link
Contributor

@watsonian watsonian commented Mar 5, 2014

I like this idea a lot. It should be possible to do using a similar technique to how the git diff info is used in the gutter, but that would only work on git-backed projects. Anything more than that would likely require lower level changes. I'll try looking into this when I have some spare time.

@sheldon
Copy link

@sheldon sheldon commented Mar 6, 2014

I've tried to implement this. I managed to get a version working based off git diff, but thought it would be a bad implementation as not every edited thing is in a git repo.

Anyone know of a way to get the edited lines in a buffer compared to what's on disk? That would be an easy drop in for my patch, and would have this feature done and working.

I've checked on the editor and buffer apis, both seem to have concepts of transactions and diffs, but I can't see any 'modifications since last save' type calls anywhere :(

@jonnott
Copy link

@jonnott jonnott commented Mar 10, 2014

+1 ... I make a single change, but Atom has snipped off all my blank lines that only previously contained whitespace.. so the diff is crazy. Can this optionally only affect newly added lines?

@substack
Copy link

@substack substack commented May 4, 2014

Please implement this feature.

Around a quarter of all the pull requests I get are full of whitespace thrashing because people's editors are configured to be really noisy. I usually just let those pull requests fester because they take much more effort to evaluate and make it harder to merge other requests later that were merged off of a prior HEAD.

Automatically fixing whitespace is really a hugely terrible idea when submitting a patch to a project that you don't control. Further, there should be a distinction between "trailing whitespace" and "leading whitespace". I personally don't get why we make a special case for indentation for empty lines. For example, this way:

if (true) {
  console.log('beep');
  // empty line, leading 2 spaces
  console.log('boop');
}

seems much more consistent than:

if (true) {
  console.log('beep');
// empty line, no whitespace because it's "trailing whitespace"
  console.log('boop');
}

because my mental model of whitespace is to show how blocks of code are organized, without a special case for empty lines.

I suspect that the former, cruder model has arisen simply because editors are really lazy about testing for "trailing whitespace" with a /\s+$/, not bothering to check that the whitespace is actually "leading whitespace" first.

@rwaldron
Copy link

@rwaldron rwaldron commented May 4, 2014

Or, people could stop being so lazy and do interactive commits wherein they stage only the lines that are relevant to their change. Why burden the editor with a task that already has a real solution?

@joshma
Copy link

@joshma joshma commented May 10, 2014

To be hyperbolic, being lazy is pretty much the essence of programming. Apart from the real issue that splitting patches in git will only go so small before you have to manually edit the patch, it's also annoying when you've already reviewed the changes via some other diff tool and are then forced to interactively add all the changes to a file because one line towards the bottom has an extra space.

Would be happy to see this implemented; thanks for creating the plugin.

@joshbetz
Copy link
Author

@joshbetz joshbetz commented May 10, 2014

Or, people could stop being so lazy and do interactive commits wherein they stage only the lines that are relevant to their change. Why burden the editor with a task that already has a real solution?

That also only works for git. What if I use SVN?

@ghelton
Copy link

@ghelton ghelton commented May 15, 2014

+1 on this, especially for the reasons listed by @substack. If don't read through every package installed by default with Atom you may miss this one. While I like the sentiment, this package is incredibly destructive with very little feedback if you're not expecting it. On top of that reading whitespace diffs with git is a pain by default since it doesn't render the whitespace characters.

@shiraleeana
Copy link

@shiraleeana shiraleeana commented May 19, 2014

+1
Eg: Sublime has an option
trim_trailing_white_space_on_save: false
Which is actually especially handy on files like Jade where the whitespace at the end is significant.

@wwoods
Copy link

@wwoods wwoods commented Jun 9, 2014

+1 very important for mixed IDE companies

@probablycorey probablycorey self-assigned this Jun 26, 2014
@jipiboily
Copy link

@jipiboily jipiboily commented Jul 8, 2014

While I would love this exact feature, this is not as self contained (requiring some sort of tie to the git package). I created a PR to add a config option to disable the package for certain paths/projects, which might serve some of you well until #8 implemented. See it here: #37.

@wwoods
Copy link

@wwoods wwoods commented Jul 8, 2014

The editor should simply take a snapshot of the file and generate a diff on
save, trimming only diffed lines. Then take another snapshot and repeat.
In my opinion, excluding paths or files manually is of little use.
On Jul 8, 2014 2:33 PM, "Jean-Philippe Boily" notifications@github.com
wrote:

While I would love this exact feature, this is not as self contained
(requiring some sort of tie to the git package). I created a PR to add a
config option to disable the package for certain paths/projects, which
might serve some of you well until #8
#8 implemented. See it here:
#37 #37.


Reply to this email directly or view it on GitHub
#8 (comment).

@jipiboily
Copy link

@jipiboily jipiboily commented Jul 8, 2014

@wwoods: fair enough. Though that was a simple solution to my problem. A better solution would be welcomed any time :)

@probablycorey probablycorey removed their assignment Jul 10, 2014
@Erwyn
Copy link

@Erwyn Erwyn commented Aug 8, 2014

👍 for this feature, much needed. I edit a quite large code base and I don't want to pollute the git diff while I don't want to introduce any new trailing space.

@dan-diaz
Copy link

@dan-diaz dan-diaz commented Oct 2, 2014

+1 - This would be a very helpful feature

@jschroeder9000
Copy link

@jschroeder9000 jschroeder9000 commented Oct 10, 2014

I think this is a very much-needed feature.

Most of us would probably agree that trailing whitespace is bad. And I understand the motivation to kill trailing whitespace by default (because trailing whitespace is bad). The problem is it's just not always practical.

I am very hesitant to use atom on legacy projects with a lot of whitespace craziness because of this. @mokkabonna mentions in #10 that one should "run a strip whitespace script on all your files, then commit just that". That's a fair assessment, but it forces an all or nothing approach, and I don't want to be forced into that. IMHO this is the perfect middle ground.

@curtisblackwell
Copy link

@curtisblackwell curtisblackwell commented Nov 13, 2014

According to this SE post, you can ignore whitespace when you run git diff.

@danjuls
Copy link

@danjuls danjuls commented Nov 26, 2014

+1 a really useful thing to have

@aguiraf
Copy link

@aguiraf aguiraf commented Dec 10, 2014

👍 very important to have

@wesbland
Copy link

@wesbland wesbland commented Dec 15, 2014

Correct, but that's much easier to track on a line by line basis instead of
buffering some arbitrary point in the past.

On Sunday, December 14, 2014, Tom Byrer notifications@github.com wrote:

When you go to a newline, [Vim] removes the extra space.

Only after changing text on that line I hope, not just arrowing though.


Reply to this email directly or view it on GitHub
#8 (comment).

@wesbland
Copy link

@wesbland wesbland commented Dec 15, 2014

I was mistaken. Vim doesn't actually trim all lines you edit, just the
blank ones (unless you touch them again). This seems like an even easier
first step that would simplify a lot of use cases. Right now, adding in
vertical whitespace is a pain because of this. I'd love to dig in and try
to help with a PR myself, but I'm so unfamiliar with the API and slammed at
work right now that it's not going to happen anytime soon.

On Sun, Dec 14, 2014 at 8:43 PM, Wesley Bland wesley@wesbland.com wrote:

Correct, but that's much easier to track on a line by line basis instead
of buffering some arbitrary point in the past.

On Sunday, December 14, 2014, Tom Byrer notifications@github.com wrote:

When you go to a newline, [Vim] removes the extra space.

Only after changing text on that line I hope, not just arrowing though.


Reply to this email directly or view it on GitHub
#8 (comment).

@wwoods
Copy link

@wwoods wwoods commented Dec 15, 2014

Ah. Well, this whole issue is stupidly simple, except node has no built-in
diff module.

Is it permissible to have the white-space plugin depend on a package from
npm?
On Dec 15, 2014 10:21 AM, "Wesley Bland" notifications@github.com wrote:

I was mistaken. Vim doesn't actually trim all lines you edit, just the
blank ones (unless you touch them again). This seems like an even easier
first step that would simplify a lot of use cases. Right now, adding in
vertical whitespace is a pain because of this. I'd love to dig in and try
to help with a PR myself, but I'm so unfamiliar with the API and slammed
at
work right now that it's not going to happen anytime soon.

On Sun, Dec 14, 2014 at 8:43 PM, Wesley Bland wesley@wesbland.com
wrote:

Correct, but that's much easier to track on a line by line basis instead
of buffering some arbitrary point in the past.

On Sunday, December 14, 2014, Tom Byrer notifications@github.com
wrote:

When you go to a newline, [Vim] removes the extra space.

Only after changing text on that line I hope, not just arrowing though.


Reply to this email directly or view it on GitHub
#8 (comment).


Reply to this email directly or view it on GitHub
#8 (comment).

@NodeGuy
Copy link

@NodeGuy NodeGuy commented Jan 25, 2015

👍

@johanlunds
Copy link

@johanlunds johanlunds commented Feb 8, 2015

I wanted this feature so I took a stab at implementing it. See PR #57. Thoughts?

@kankaristo
Copy link

@kankaristo kankaristo commented Jun 9, 2015

I have to work with a horribly written library, with plenty of trailing whitespace. On the rare occasions I have to edit it, I usually edit it outside of Atom because of the whitespace stripping for the entire file.

For a moment, I thought this is exactly what I wanted (and it is a nice feature), but then I realized that for my own code, I actually want to strip whitespace for the entire file. I just want to disable it for some paths.

This can be done in init.coffee with something like this (you could flip this around to enable for some paths instead):

# Observe text editors to disable whitespace stripping for certain paths
atom.workspace.observeActivePaneItem (paneItem) ->
  return unless path = paneItem?.getPath?()

  ignorePaths = [
    "path/to/horribly/written/library/redacted/"
  ]

  for ignorePath in ignorePaths when path.indexOf(ignorePath) > -1
    # Disable stripping of trailing whitespace
    atom.config.set("whitespace.removeTrailingWhitespace", false)
    console.log "Disable stripping of trailing whitespace"
    return

  # Enable stripping of trailing whitespace
  atom.config.set("whitespace.removeTrailingWhitespace", true)
  console.log "Enable stripping of trailing whitespace"

Edit: fixed bug when pane is empty.

Still, +1 for this feature (much simpler than adding enabled/disabled paths to init.coffee).

@emecell
Copy link

@emecell emecell commented Oct 20, 2015

I agree with everyone else, this would be great to have.

@jasdeepkhalsa
Copy link

@jasdeepkhalsa jasdeepkhalsa commented Dec 14, 2015

+1

1 similar comment
@TheAxnJaxn
Copy link

@TheAxnJaxn TheAxnJaxn commented Jan 11, 2016

+1

@sorcamarian
Copy link

@sorcamarian sorcamarian commented May 30, 2016

I disable the the whitespace package when I perform such a edit.

(package comes with atom)

@simoncpu
Copy link

@simoncpu simoncpu commented Jun 28, 2016

Let me add to noise by posting another "+1" Hehehe...

@abudayah
Copy link

@abudayah abudayah commented Jul 11, 2016

+1

@rsweetland
Copy link

@rsweetland rsweetland commented Jul 26, 2016

This seems to be a good "bipartisan" resolution to #10. Atom can still make the world a more white-space friendly place, one line at a time.

@mohsaied
Copy link

@mohsaied mohsaied commented Jan 4, 2017

+1

@esmail
Copy link

@esmail esmail commented Sep 12, 2017

A bit stale in here but I still think this would be wonderful to fix.

As a motivating example, I just edited a few lines of a MarkDown file, then Atom fixed whitespace issues in about 70 more lines that I haven't necessarily read and don't want the latest blame on.

@jaller94
Copy link

@jaller94 jaller94 commented Oct 13, 2017

+1 The Git integration of Atom not displaying whitespaces made this irritating.

I like the idea of having a string value like proposed in microsoft/vscode#13157.
removeTrailingWhitespace: "all" | "modified" | "off"

true and false could be interpreted as "all" and "off" for backwards compatibility.

Or revive this PR: #57.

@kparaju
Copy link

@kparaju kparaju commented Nov 8, 2017

+1

@BARJ
Copy link

@BARJ BARJ commented Nov 27, 2017

+1

1 similar comment
@carlos-algms
Copy link

@carlos-algms carlos-algms commented Dec 11, 2017

+1

@Arcanemagus
Copy link
Member

@Arcanemagus Arcanemagus commented Dec 11, 2017

With the (somewhat recent) changes to TextBuffer, this should be pretty simple to implement:

  1. Use TextBuffer::createCheckpoint to save a checkpoint
  2. Use TextBuffer::getChangesSinceCheckpoint to get a grouped list of changes since the checkpoint
  3. Only run the whitespace stripping on the lines from the grouped changes

The only semi-difficult part of this is determining when to save the checkpoint. I think doing so when first opening a TextEditor, then on each time it is saved will suffice. The potential issues I see with that are:

  • Do old checkpoints need to be manually deleted? There is no public method to do so
  • Is this actually often enough to catch all changes made by a user?
  • What to do when the checkpoint is no longer valid? Fall back to VCS diffs like was done in #57?
@stevenpetryk
Copy link

@stevenpetryk stevenpetryk commented Dec 15, 2017

@Arcanemagus:

  • Checkpoints should get garbage collected like any other object—once nobody carries a reference to a checkpoint, it's gone.
  • That should be suitable I'd imagine, right? If the checkpoint system really aggregates all the changes it should work just fine.
  • If the checkpoint isn't valid, I think just doing a noop would be the safest and least surprising behavior. It's probably best not to get VCS involved here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet