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

Updating code conventions on line length #3471

Closed
zapashcanon opened this issue Mar 31, 2017 · 7 comments
Closed

Updating code conventions on line length #3471

zapashcanon opened this issue Mar 31, 2017 · 7 comments

Comments

@zapashcanon
Copy link
Contributor

Hi,

To close #3071 I started #3470. So I'd like to stay as close as possible to the current conventions, as @jasp00 advised.

Unfortunately, there are two main things in the current coding conventions which should be changed, one of them is line length.

Currently, conventions say we should avoid length > 80.

First thing is, it's not an absolute requirement, as stated in the conventions ; and indeed we have 211 char. lines.

So, I'd like us to talk about this so we can decide what we want.

I guess there are two cases for breaking lines. First one is for things like:

char* txt = "Here is a very looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooonnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnng line";

IMHO, we shouldn't break lines in this case, because almost every text editor will display it in a convenient way, which would be better than what we could do with a tool to format it for us, line breaking is useless then. (I use atom and nvim, I'm not sure if all editor used by LMMS contributor is doing this, so, sorry if it's not the case..)

In atom e.g.:

capture du 2017-03-31 19-33-14

Second case is when line break really improves readability, for things like:

char** txt = {
    "first line",
    "second line",
    // etc. until it doesn't fit on one line
    "last line"
};

In this case, the fact is that if you let in on a single line, the editor isn't smart enough to display it in a convenient way:

capture du 2017-04-01 01-18-31

So in this case, I believe it's better to break line. Good thing is, in this case, tools like astyle will be able to do proper indentation for you.

So my proposal is that we stop asking people to break line just to avoid go over 80 char. But asking to break line only when it really improves readability. And we tell KWStyle not to check for line length (or, just to check for something really big like 1000 char to detect problems / abuse).

So, now, I'll @ contributors who probably have something to say about this. Sorry for this... (hoping no one's is missing).

Main contributors: @Wallacoloo, @lukas-w, @softrabbit, @jasp00, @Umcaruje, @BaraMGB, @grejppi, @michaelgregorius, @zonkmachine, @liushuyu, @tresf

New contributors: @karmux, @Spekular, @PaulBatchelor, @follower, @DeRobyJ , @StCyr

the ones that no longer contribute directly to codebase but would love to hear from
@tobydox, @diizy, @Fastigium, @curlymorphic, @badosu

If some of you don't wan't to talk about this, sorry for this. But please, tell me, so I won't @ you in others issues to discuss that kinda changes.

@Wallacoloo
Copy link
Member

Wallacoloo commented Mar 31, 2017 via email

@zapashcanon
Copy link
Contributor Author

zapashcanon commented Mar 31, 2017

Edit: before my edit, numbers were wrong...

So, I just checked for .cpp .hpp .h .c files in src, tests and include:

Number of lines > 80 char: 1415
Number of lines > 100 char: 358
Number of lines > 120 char: 90
Number of lines > 150 char: 9
Number of lines > 200 char: 1

@Spekular
Copy link
Member

Spekular commented Apr 1, 2017

I'd prefer if we don't break lines to hit an arbitrary character limit, but I really don't mind either way so I'll leave this to everyone else.

@jasp00
Copy link
Member

jasp00 commented Apr 1, 2017

I don't know anyone who actually codes in an environment constrained to 80 characters on a line.

I do.

I work with 80-char terminals and command-line tools. I use nano editor, among others. It is not a matter of one single editor, if you drop the 80-char wrap, you make the available tools harder to use.

Your example does not display well in gedit and Evolution. Comparing a diff of long lines is very difficult.

Long lines are hard to follow on the GitHub interface; it is difficult to see file changes. You can see that your example requires a scroll bar; compare with:

char* txt = "Here is a very loooooooooooooooooooooooooooooooooooooooooooooooooo\
ooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo\
oooooooooooooooooooooooooooooooooooooooooooooooooooooonnnnnnnnnnnnnnnnnnnnnnnnn\
nnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnng \
line";

If your editor were smart enough, you would not need to worry about wrapped lines.

@tresf
Copy link
Member

tresf commented Apr 1, 2017

Long lines are hard to follow on the GitHub interface; it is difficult to see file changes.

I agree that often the changes run off the right side of the screen when this occurs however strict 80-char adherence often causes the diffs to start to span unnecessary newlines when just a few characters are added. In that case, the diff shows several lines modified instead of a single line. The GitHub review process is burdened by both scenarios.

@jasp00
Copy link
Member

jasp00 commented Apr 1, 2017

if you can live with no (mandatory) wrapping, we can make that call now and close out #3471.

"Not an absolute requirement – sometimes longer lines can't be avoided. But it is a friendly thing to do."

80-char wrapping is a friendly thing to do. I can live with whatever coding conventions, but you will make my review and coding tasks harder.

@zapashcanon
Copy link
Contributor Author

I'm closing, will do it another way.

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

No branches or pull requests

5 participants