lewang / ws-butler

Unobtrusively trim extraneous white-space *ONLY* in lines edited.
242 stars 26 forks source link

Assorted gripes #1

Closed dgutov closed 11 years ago

dgutov commented 11 years ago
  1. unobstrusively has an extraneous s inside.
  2. (require 'ws-butler) signals error "Required feature was not provided".
  3. If the line has trailing whitespace and the point is at the end of it when the buffer is saved, that whitespace is not cleared.
  4. No replacing of tabs with spaces or removing trailing newlines.

Overall, I like how small the package is. Good idea reusing highlight-changes-mode.

lewang commented 11 years ago
  1. fixed.
  2. fixed.
  3. This is a feature. Part of the unobtrusiveness. The space is actually trimmed on disk.
  4. With tabs, I assume the user control the insertion of tabs with indent-tabs-mode, is there a situation where this is not true? You mean trailing new lines in the file?
dgutov commented 11 years ago

3) Huh. Nice. 4) You can paste a piece of pre-indented code that uses tabs, or you can edit a file that's been indented with tabs with the intention of converting tabs to spaces piece-by-piece, without dirtying the commits with unnecessary changes. Trailing newlines in the file, yes. ethan-wspace does both of these things.

lewang commented 11 years ago

How's that now?

lewang commented 11 years ago

Note that I only implemented untabifying for leading tabs. Not sure if it's a great idea to touch text not a part of the indentation.

dgutov commented 11 years ago

Saving while having a line with just indentation signals an error:

Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
  =(nil 9)
  (if (= (char-after) 9) (progn (untabify (point) (progn (skip-chars-forward "  " (point-at-eol)) (point)))))
  (let ((eol (point-at-eol))) (skip-chars-forward " " (point-at-eol)) (if (= (char-after) 9) (progn (untabify (point) (progn (skip-chars-forward "  " (point-at-eol)) (point))))))
  (if indent-tabs-mode nil (let ((eol (point-at-eol))) (skip-chars-forward " " (point-at-eol)) (if (= (char-after) 9) (progn (untabify (point) (progn (skip-chars-forward "     " (point-at-eol)) (point)))))))
  (while (not (eobp)) (if indent-tabs-mode nil (let ((eol (point-at-eol))) (skip-chars-forward " " (point-at-eol)) (if (= (char-after) 9) (progn (untabify (point) (progn (skip-chars-forward "     " ...) (point))))))) (end-of-line) (delete-horizontal-space) (forward-line 1))
  (save-restriction (narrow-to-region beg end) (goto-char (point-min)) (while (not (eobp)) (if indent-tabs-mode nil (let ((eol (point-at-eol))) (skip-chars-forward " " (point-at-eol)) (if (= (char-after) 9) (progn (untabify (point) (progn ... ...)))))) (end-of-line) (delete-horizontal-space) (forward-line 1)))
  (save-excursion (save-restriction (narrow-to-region beg end) (goto-char (point-min)) (while (not (eobp)) (if indent-tabs-mode nil (let ((eol (point-at-eol))) (skip-chars-forward " " (point-at-eol)) (if (= (char-after) 9) (progn (untabify ... ...))))) (end-of-line) (delete-horizontal-space) (forward-line 1))))
  ws-butler-clean-region(89 104)
  (lambda (_prop beg end) (save-excursion (setq beg (progn (goto-char beg) (point-at-bol)) end (progn (goto-char end) (point-at-eol)))) (if (and (>= (point) beg) (<= (point) end)) (progn (setq ws-butler-presave-coord (list (line-number-at-pos (point)) (current-column))))) (ws-butler-clean-region beg end) (setq last-end end))(hilit-chg 99 104)
  funcall((lambda (_prop beg end) (save-excursion (setq beg (progn (goto-char beg) (point-at-bol)) end (progn (goto-char end) (point-at-eol)))) (if (and (>= (point) beg) (<= (point) end)) (progn (setq ws-butler-presave-coord (list (line-number-at-pos (point)) (current-column))))) (ws-butler-clean-region beg end) (setq last-end end)) hilit-chg 99 104)
  (if prop (funcall func prop start (or end limit)))
  (while (and start (< start limit)) (setq prop (get-text-property start (quote hilit-chg))) (setq end (text-property-not-all start limit (quote hilit-chg) prop)) (if prop (funcall func prop start (or end limit))) (setq start end))
  (let ((start (or start-position (point-min))) (limit (copy-marker (or end-position (point-max)))) prop end) (while (and start (< start limit)) (setq prop (get-text-property start (quote hilit-chg))) (setq end (text-property-not-all start limit (quote hilit-chg) prop)) (if prop (funcall func prop start (or end limit))) (setq start end)))
  ws-butler-map-changes((lambda (_prop beg end) (save-excursion (setq beg (progn (goto-char beg) (point-at-bol)) end (progn (goto-char end) (point-at-eol)))) (if (and (>= (point) beg) (<= (point) end)) (progn (setq ws-butler-presave-coord (list (line-number-at-pos (point)) (current-column))))) (ws-butler-clean-region beg end) (setq last-end end)))
  (let (last-end) (ws-butler-map-changes (function (lambda (_prop beg end) (save-excursion (setq beg (progn (goto-char beg) (point-at-bol)) end (progn (goto-char end) (point-at-eol)))) (if (and (>= (point) beg) (<= (point) end)) (progn (setq ws-butler-presave-coord (list ... ...)))) (ws-butler-clean-region beg end) (setq last-end end)))) (if last-end (progn (goto-char (point-max)) (skip-chars-backward "  \n") (let ((printable-point-max (point))) (if (>= last-end printable-point-max) (progn (ws-butler-trim-eob-lines)))))))
  ws-butler-before-save()
dgutov commented 11 years ago

Not sure if it's a great idea to touch text not a part of the indentation

That should be fine. Non-indentation tabs can be converted by hand.

Regarding 3) again, I think that it may be worth only doing when the line has nothing but indentation (to save the user the trouble of re-indenting), but not when there is some text on the line. I don't remember ever saving the buffer after intentionally typing space. Not sure about this, just a thought. But as pertaining to trailing newlines, I having the visible feedback that lines are being cleaned is more useful than keeping point where it was.

dgutov commented 11 years ago

On tabs again, if the point is just after a tab character or several, after cleanup it moves before them.

lewang commented 11 years ago

On tabs again, if the point is just after a tab character or several, after cleanup it moves before them.

Does this still repro? I couldn't get it to.

but not when there is some text on the line. I don't remember ever saving the buffer after intentionally typing space.

Well if you never save a file after pressing space, there is no problem. I just think it's easier to explain ws-butler's behaviour as you do your work, and the butler cleans up your spaces without you knowing the butler was ever there.

dgutov commented 11 years ago

Does this still repro?

It does. With this file and ws-butler-mode enabled:

def foo
  bar
end

(goto-line 2) C-q TAB C-x C-s

Well if you never save a file after pressing space, there is no problem.

Maybe you're right about this case.

the butler cleans up your spaces without you knowing the butler was ever there.

I see the idea, but I think the main (only?) practical benefit is that you don't need to reindent the current empty line right after saving. The downside is you don't see the buffer the way it's saved, and that's bad if the trailing whitespace is actually significant, such as the case with yasnippet snippet files. The user might feel obligated to clean the already removed whitespace manually. Maybe add an option here?

lewang commented 11 years ago

On Mon, Jan 14, 2013 at 6:46 AM, Dmitry Gutov notifications@github.comwrote:

Does this still repro?

It does. With this file and ws-butler-mode enabled:

def foo bar end

(goto-line 2) C-q TAB C-x C-s

Fixed.

the butler cleans up your spaces without you knowing the butler was ever there.

I see the idea, but I think the main (only?) practical benefit is that you don't need to reindent the current empty line right after saving. The downside is you don't see the buffer the way it's saved, and that's bad if the trailing whitespace is actually significant, such as the case with yasnippet snippet files.

This is why I originally didn't include a globalized minor-mode. My thought was that it's better to allow people to decide to use this for each major-mode they use.

The user might feel obligated to clean the already removed whitespace manually. Maybe add an option here?

I'll think about this.

Le

dgutov commented 11 years ago

Fixed.

Works fine, thank you.