roryokane / detectindent

Vim script for automatically detecting indent settings
21 stars 6 forks source link

Do not set shiftwidth=0 #4

Closed blueyed closed 9 years ago

blueyed commented 9 years ago

It's useful to have different values for shiftwidth and tabstop, especially with noexpandtab.

I have just changed my default for tabstop to 8, but want to use shiftwidth=2. I also use set expandtab by default.

roryokane commented 9 years ago

Note that shiftwidth was set even in ciaranm’s original detectindent. My change with shiftwidth was to set it to 0 (only when 0 is supported) instead of always directly to the detected number of spaces. I changed the value of shiftwidth, but not the fact that it was set. Even with the old behavior, if you wanted shiftwidth to be different from tabstop, you would have to set it manually after running :DetectIndent.

I think that most people want shiftwidth to equal tabstop. I can see four options for allowing that while supporting different values:

Never set shiftwidth

You would set shiftwidth=2 in your vimrc. Most people would set shiftwidth=0 in their vimrc.

The problem with this is that a 0 value for shiftwidth is not supported in Vim versions below 7.4, which some people might be stuck on. So with this behavior, some people would always have to set shiftwidth manually to the detected tabstop value, defeating part of the purpose of the plugin.

Only set shiftwidth if it doesn’t support a 0 value

I could have the plugin detect whether this Vim supports a 0 value, and change shiftwidth only if it doesn’t support 0. That would make the plugin still valuable for people on old Vim, while giving people freedom on newer Vims. But I think the inconsistency would be confusing for people who upgrade Vim – suddenly, shiftwidth stops being set right and the plugin asks them to add something to their vimrc. “I thought upgrading was supposed to make things better,” they would complain.

A secondary problem for both of the above options is that they requires people to set shiftwidth in their vimrc, which perhaps not everybody does. And most people who set it probably haven’t heard of the 0 value required to make it work always, so they will still be frustrated if they set it to 4 and load a file with 2-space indents. We could educate them in the README for this plugin – put changing their vimrc in the instructions – and that would be the best compromise, but it would still require people to do more work to install this than they do today.

Leave shiftwidth alone only if some option is set

I don’t see a way to automatically detect from the user’s settings whether they want shiftwidth to be left alone. Thus, I would have to add a new option, something like g:detectindent_use_global_shiftwidth.

Most of the time, the plugin would set shiftwidth. But if that option exists and is set to 1, it would leave shiftwidth unchanged.

I think this would work. It would make the code a little more complex, and add a new option, but that seems unavoidable.

Current situation – you change it manually

Or if the value for shiftwidth that you want isn’t always 2 – if it is different for different files, while also being different from tabstop sometimes – then your only option is to run :DetectIndent, and then run :set shiftwidth=3 or whatever.

From these four choices, I am guessing you would prefer adding a Vim option to leave shiftwidth alone. Is that correct? Do you disagree with any premise? Do you see any other options, or reasons to choose an option that I have missed?

mMontu commented 9 years ago

I liked the idea of shiftwidth=0 -- I found it really easier to manually correct the indentation. I've never found a situation where it would be useful to have different values for shiftwidth and tabstop like blueyed mentioned (but my knowledge on this area is very limited).

This approach does break some plugins that are based on &sw instead of shiftwidth(), tough. At first I thought it would be easy to correct them -- for instance, the vimwiki plugin (https://github.com/vimwiki/vimwiki/pull/98) was changed accordingly. Them I noticed that the indent operator '=' was broken for several languages. A grep for &sw on $VIMRUNTIME/indent/ showed that the default indent for 62 languages are based on &sw. So maybe it will be easier to avoid sw=0.

It might be better to have the plugin to set both shiftwidth and softtabstop to sensible values. To make it easy to to manually correct indentation a new command could be created, which change all related options together,

roryokane commented 9 years ago

@blueyed’s issue has been moved to issue #9, “Allow setting tabstop and shiftwidth differently”. This issue, #4, is now only about whether and when to set shiftwidth to 0, which @mMontu’s comment is about.

roryokane commented 9 years ago

@mMontu suggested never setting shiftwidth to 0, and creating a new command with which the user can change shiftwidth, softtabstop, and tabstop simultaneously. But I think I would prefer introducing a new option like g:detectindent_use_zero_shiftwidth, and not creating a new command. When the option is 1, shiftwidth would be set to 0, and when the option is 0, shiftwidth would be set to whatever number of spaces is detected.

The reason I would prefer an option, instead of always using a nonzero shiftwidth, is that I would personally set it to use shiftwidth=0. Even though I occasionally encounter non-updated plugins that break with that setting, I am okay with that, because it helps me notice that those plugins have a problem. Then I can fix the plugin myself and improve the Vim ecosystem. But most users don’t want to be forced into that responsibility, so they can set the option to not use shiftwidth=0.

With this option solution, users who really care about changing the indent size conveniently would choose shiftwidth=0, and live with the incompatibilities. Alternatively, it would still be possible to define a command to set all three options at once like you suggested, only when they ask for a nonzero shiftwidth. But I think such a command seems a little out of scope for this plugin, “DetectIndent”. I can imagine that the user might already be using a different plugin that is useful on its own, “ChangeIndentEasily”, so it would be redundant to define our own command for that.

g:detectindent_use_zero_shiftwidth needs to have a default when it is undefined. The default could be 0, so that most people’s indent commands still work even when they use non-updated language plugins that rely on 'shiftwidth' instead of shiftwidth(). Or the default could be 1, so that most people can change the indentation very conveniently whenever the plugin detects the indentation size incorrectly. Which is more important – compatibility with certain plugins, or convenience when correcting indent size?

I think compatibility is more important, since it might be hard for the uneducated user to debug the problem if a language plugin is failing because 'shiftwidth' is 0. Whereas if a user is really annoyed at the inconvenience of changing the options, they will notice shiftwidth=0 in the documentation for 'shiftwidth', and figure out that they can set this plugin’s option for that. So the default of g:detectindent_use_zero_shiftwidth should be 0.

softtabstop is currently set to -1 when possible, which gives it a similar behavior to shiftwidth=0. This value is probably okay to keep, even when g:detectindent_use_zero_shiftwidth is 0, because neither me nor @mMontu have observed plugins that break if softtabstop’s value is non-negative.

Does anyone have any objections or new alternatives to introducing an option g:detectindent_use_zero_shiftwidth? And is that a good name for the option?

mMontu commented 9 years ago

I agree that this plugin may not be the better place for the new command, and that the default for the new option should be 0 (compatibility over convenience).

I think the name of the variable is OK; what is not clear for me is why it is useful to keep setting 'shiftwidth' to zero, even as an option. My understanding is that manually correcting the indentation settings should be less frequent as the plugin evolves and set them all to proper values automatically.

Thus the benefits of shiftwidth=0 will continuously drop over the time; the overhead of changing unrelated plugins to use shiftwidth() won't pay off. And it will increase the complexity of the plugin, as it may work (detect the indent) correctly on some files for a given value of the new option and fail for the other value.

But again, my knowledge on the several indent options is quite limited, so I'm probably oversimplifying the problem.

blueyed commented 9 years ago

I just came across what @mMontu noticed: even Vim's runtime/indent/vim.vim does not use shiftwidth() but &sw, resulting in ={motion} not working properly (with indentexpr=GetVimIndent()).

I've just noticed this after updating the plugin and stashing my local changes. How can this be useful / not annoying for you?

Even though I occasionally encounter non-updated plugins that break with that setting, I am okay with that, because it helps me notice that those plugins have a problem. Then I can fix the plugin myself and improve the Vim ecosystem.

How about sending a patch to the various maintainers of Vim's runtime files then? ;)

:h 'sw' says:

> When zero the 'ts' value will be used.

So for the DetectIndent command I'd say the only sane behavior appears to be simulating that. And if you manually want to change sw (and ts), you would have to use set sw=X ts=X instead of only set ts=X.

roryokane commented 9 years ago

@blueyed

How can [0 shiftwidth] be useful / not annoying for you?

Actually, I already agree – I have gradually changed my mind on whether it’s worth suffering buggy plugins just so I can fix them upstream. Just a few days ago, I had indentation problems when editing Clojure code because of this type of bug in the Clojure indenter. And the outdated indent plugins bundled with Vim are also a problem, since I have heard that getting changes into Vim itself is a big hassle. So I am giving up on fixing the Vim ecosystem.

I had been dragging my heels on this change because I wanted to do another change first, improving the indent detection algorithm. But I guess I should do this first so that editing is less buggy for everyone in the meanwhile.

roryokane commented 9 years ago

I decided not to bother implementing my proposed option g:detectindent_use_zero_shiftwidth, since I don’t want it anymore. But if anyone later thinks they would actually use that option, feel free to make a new issue asking for it.

blueyed commented 9 years ago

@roryokane Thanks for addressing this!

mMontu commented 9 years ago

It works like a charm, thanks!