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

Two miscellaneous improvements for Vim #15450

Merged
merged 2 commits into from
Jul 21, 2014

Conversation

chris-morgan
Copy link
Member

No description provided.

setlocal expandtab
" Why 79? See http://aturon.github.io/style/whitespace.html and
" https://github.com/aturon/rust-guidelines/pull/12.
setlocal textwidth=79
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression that Rust was actually 99 characters, and that @aturon's whitespace style guide wasn't up to date.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100 used to be the recommended figure, but more recently it has become 80.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with this. Even ignoring the 80 vs 100, textwidth doesn't work very well for programming because it breaks lines on whitespace, which is not necessarily the correct point to break a line of code.

Also, why 79? You'd want to say 80 if you want 80-length lines, not 79.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better solution here would be instead setlocal colorcolumn=80,100. That will highlight those two columns, without otherwise affecting the behavior of entering text. So it allows users to see where the two lengths are and they can make their own decisions about how best to wrap their lines.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kballard 'textwidth' is just fine for this, and 79 is the correct value, not 80, for the reasons indicated in the issue given.

I do not see what the position in which it breaks thing has got to do with it. Sure, it may go wrapping your code in a way you don’t like if you’re not careful, but that will push you in the direction of fixing it up properly.

Personally I find the most useful thing about setting 'tw' is that you can use gq on comments.

I do not believe that 'colorcolumn' is an appropriate solution, for multiple reasons. (a) it is hated by some people as a real ugliness, especially in colorschemes that have not optimised it at all (we’d earn enemies by setting it, and there is no precedent for doing it ourselves that I know of), and (b) it’s only supported for Vim 7.3 and later. As it happens, I do have ccu=+1,+21, but I do not think it would be a good idea to put it here. But if we did set 'ccu', I would want 'tw' set too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I forgot that you could set a format option that disables wrapping on non-comments on insert mode. That makes textwidth a lot more appropriate.

As for 79 vs 80, neither link actually explains it. The first link says nothing and the second is you asserting that "terminals will tend to need an extra, blank line if you write 80-character-wide lines". But why are you asserting that? I just did a quick test and my CLI vim does in fact show a 1-width gutter always, so it can only fit 79 characters on a line in an 80-character terminal, but if that's what you're referring to, I'm not convinced that matters, because who actually uses 80-character-wide terminal windows these days?

You referred to PEP 8, which makes a similar claim that "editors with the window width set to 80" will wrap an 80-character line, but I don't see any explanation there for that claim either. Vim works just fine when the number of characters on a line is equal to the line width (i.e. window width - the gutter that is apparently 1 char).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I just did a quick test of emacs, and I get the feeling that emacs is the reason for this. In my 94-character-wide terminal, emacs is happy to display 93 characters, but it wraps the 94th to the next line even though it could put it on the previous line. It chose to add a wrap indicator to the end of the previous line instead.

So that's pretty stupid.

But the question remains, who still uses 80-character-wide terminals, and why are we optimizing for them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command Prompt on Windows is one with 80-character-wide terminals, though if you look carefully enough you can figure out how to alter it. In my Windows days I normally had a 120-character terminal there, but I’ve never seen anyone else with other than 80.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do people actually edit files in Command Prompt?

@chris-morgan
Copy link
Member Author

I’ve removed the third commit as the now-landed #15476 has covered its effects entirely, and rebased the rest for a tiny merge conflict.

What remains should be non-controversial.

r?

@chris-morgan chris-morgan changed the title Three miscellaneous improvements for Vim Two miscellaneous improvements for Vim Jul 18, 2014
syn match rustModPathSep "::"
" rustModPathInUse is split out from rustModPath so that :syn-include an get the group list right.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment typo, "can" instead of "an".

@lilyball
Copy link
Contributor

r=me with my comments addressed.

Here’s what the Vim manual says in *:syn-include*:

    :sy[ntax] include [@{grouplist-name}] {file-name}

	All syntax items declared in the included file will have the
	"contained" flag added.  In addition, if a group list is
	specified, all top-level syntax items in the included file will
	be added to that list.

We had two rules for `rustModPath`, one `contained` and the other not.
The effect was that the second (now renamed to `rustModPathInUse`) was
being included in the group list, and thus that all identifiers were
being highlighted as `Include`, which is definitely not what we wanted.
@chris-morgan
Copy link
Member Author

Both concerns fixed. r? (I’m not a reviewer.)

@bors bors closed this Jul 21, 2014
@bors bors merged commit 0a0c6da into rust-lang:master Jul 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants