search

purcell / whitespace-cleanup-mode

In Emacs, intelligently call whitespace-cleanup on save
125 stars 8 forks source link

Don't turn on whitespace-cleanup-mode for read only buffers #10

Closed YorkZ closed 7 years ago

YorkZ commented 8 years ago

Hi Steve,

I guess it makes more sense to not enable this mode in read only buffers. What's your opinion?

Thanks,

York

purcell commented 8 years ago

Maybe, but I'm pretty wary of that generalisation. Wouldn't some diff buffers etc. be read-only?

wyuenho commented 7 years ago

But why would you run whitespace cleanup on diff buffers?

purcell commented 7 years ago

Did you guys actually experience an issue with this, ie. you found whitespace-cleanup-mode was enabled inappropriately at some point?

wyuenho commented 7 years ago

Haven't run into any issues yet, but there's no harm in this PR at all, you can't clean up the buffer if it's read only, so there's no point in turning WSC on. Less work to do, more efficient emacs. The only problem I see with this PR is, if I toggle read-only-mode off, WSC mode doesn't turn on because there's not a callback added to read-only-mode-hook. Might make sense to add a hook there.

purcell commented 7 years ago

That fix doesn't quite feel right to me, for the reason you mention -- it's unusual to make decisions about when to enable modes based on buffer attributes (e.g. writability), because they can change.

If this were really an issue, the safer fix would probably be to make whitespace-cleanup-mode do nothing at save-time if the buffer is read-only.

Yes, this would be easy to merge, but unless there's an actual issue in general usage, I'd rather not add extra code.

YorkZ commented 7 years ago

Pretty sure I had some sort of issues with whitespace-cleanup-mode in read only buffers which was why I sent this PR. Unfortunately I didn't document those context with this PR at that time. Another lesson learned. Thanks!

purcell commented 7 years ago

Cool, thanks. I'm going to close this for now, but if concrete issues arise later, just let me know.