Now, most of these offenses include using double quotation marks instead of single quotes (when not interpolation), not following the 2 spaces per level rule, exceeding the 80 character line length rule, or using { and } for multi-line blocks.

Although they are small and easy to fix, is it appropriate to change the coding style of an open source project by fixing the offences and making a Pull Request? I acknowledge that some projects, like Rails, do not accept cosmetic changes and some are just too large to "fix" all at once (Rails, for example, generates over 80,000 offences when Rubocop is run—regardless, they have their own small set of coding conventions to follow when contributing). After all, the Ruby Style Guide is there for a reason, together with tools like Rubocop.

People appreciate consistency, so making these kinds of changes is kind of doing a good thing for the Ruby community in general, right?

Isn't not following community coding style conventions and best practices basically encouraging bad practices?

Ask the maintainers

Coding style is a quite subjective discussion, and rules like "maximum line length of 80 characters" are not always hard and fast. While general agreement should be that shorter lines are better to read, 80 might be too restrictive for some with today's screen sizes and IDE's.

Other rules can be ignored on purpose, too. For instance, a developer might consider global use of double quotes better for him and be willing to accept the "risk" of accidental interpolation and an extremely small increase on parsing time.

Many maintainers also don't like large coding style changes as they are boring to review and there is a chance that it might introduce errors. For example, a string could be switched to single quote, even though it contained a deliberate interpolation and should have been using double quotes. Maintainers prefer to do style cleanups while working on that actual code so they can verify style changes don't introduce new bugs.

Show me the code

You seem to be motivated largely by respect for the authority of the rubocop tool and the Ruby Style Guide, which the maintainers may not share. They already have their own style and are used to it, so any change would affect everyone working on the project, and that's a lot of work, especially if the project is large.

Consider the motivations of the maintainers. They (probably) want new people to join them by submitting bugfixes, bug reports, working code, and well-written documentation. If you show up and say "You have offences in rubocop" they aren't going to think "Oh good, someone new to help share the load," they're going to think "Who is this guy? Why is he telling us what to do?"

Open source projects tend to be meritocracies: you get respect based on the quality of your work. If you show up and do great things and then bring up your style concerns, they are more likely to listen, although they still might say no. There is a open source saying that goes: "Talk is cheap, show me the code."

Roll your own

You can pull these changes only if there is an open issue to fix the formatting. Otherwise start your own branch, and if the author sees that more people use your branch simply because it's more readable, they'll merge in the branch on their own, but be prepared to maintain your branch by merging in updates and constantly fixing the formatting.

If the project isn't important enough to you to keep maintaining your own branch, then it wasn't worth cleaning up in the first place.

Pull requests can be taken personally by the author. It's not a mechanism to offer criticism, and reformatting all the code could be taken as criticism.

If you don't want to maintain your own branch, but you want to contribute to a project, open a new issue and describe why the current format is causing you problems, then offer to resolve the issue for the author. If the author agrees, then they'll assign the issue to you and you now have permission to make a pull request.

You've touched on a subject that I also agree is a rampant problem on GitHub. Formatting aside, there are a number of projects that incorrectly use annotations and that causes havoc with many IDEs. I can think of three largely popular projects that use deprecated flags incorrectly and propagate warning messages in my IDE. I've sent pull requests to fix them, but the authors don't use the same IDE, thus the pull requests are ignored.

Branching, merging and fixing seem to be the only solution.

Unnecessary Nitpicking

Pragmatism over Dogma, always. Coding style guides are an especially insidious form of evil that draw attention away from architectural concerns towards frivolous nonsense like single/double quoting. Ask yourself: Does it really make a difference?

They can be good up to a point, but the second you treat them with an almost religious fervor, you've gone too far. They're guidelines, suggestions, opinions, NOT facts.

Should they just be ignored then? No, there is merit to using the tools to get a general idea of what needs to be looked at, but no more.

It's worth adding that reformatting changes tend to create merge conflicts which is a very good reason for most maintainers to hate reformatting just for sake of it.

Find more answers or leave your own answer at the original post. See more Q&A like this at Programmers, a question and answer site for professional programmers interested in conceptual questions about software development. If you've got your own programming problem that requires a solution, login to Programmers and ask a question (it's free).

And I am completely for general guidelines and nice code. It just becomes insufferable when stupid rules are taken much too seriously. The important question is not if you kept your rows below 80 characters (which is an idiotic rule btw.) But if the code is more readable when you follow the rules or not.

While I try to conform to a certain coding style and best practices, restyling someone else's code might (1) introduce necessary errors and (2) confuse the original code owner. For instance, the person might have a larger or smaller monitor screen. Restyling the code to either 60 char per line or 160 char per line is quite arbitrary either way you look at it. Even the use of braces, for example, if not something I would change even though I always use braces for a single line of code. Of course, unless you are working on the codes, then you should be allowed to restyle it to suit you needs (better readability, etc).

There are two obvious answers to this, which surprise me were not given.

1) Add an issue/bug asking if there is interest in this kind of cleanup. Be specific about why it would benifit the project. Perhaps one of the warnings actually exposes a flaw.

2) Create a pull request that simply runs the linter. What would be better is if you created some configuration file that alliviates style decisions the project already made, example would be 3 space instead of two.

With 1 you are expressing your interest with out asking for a lot of the teams time in response. If the team is happy with idea then your time won't be wasted in doing the clean up. In 2 you may still hit pushback but you are clearly expressing the interest of making the project better in the long term, again the amount of work you put in is minimal and the amount of review time for the project is small. In addition by pre configuring the linter to adhere to the projects current style you are showing that you respect the team and it's past decisions and simply went to help things stay consistent.

Personally, it would irritate my profusely if somebody only submitted a patch to one of my projects to change my style. I'm primarily a C and ASM guy, and for C I use Allman style braces and indenting. I also tend to put my literals on the left side of equality tests to protect against assignment errors. Some people are greatly irritated by either or both of these. However, typically I'm either the only developer working on my projects or I'm one of a very small group. If the only contribution you had was messing up my braces and therefore making the code less readable for those doing the actual work (me), I'm either going to ignore you if I'm feeling nice or very tersely tell you to go away.

There isn't some great style guide, written in stone, that will cause the Coding Gods to strike you dead if you don't follow. Style is - at a root level - very personal as to how your mind sees and reads code. (Okay, sometimes employers have style requirements, and you won't be struck dead, but possibly find yourself less employed.) When you move up to working in teams, then the style has to be some sort of compromise that works for most of the folks. Are there things that work for far more people than not, and therefore become a more widely accepted style? Sure. Is anything that compiles, works, and can be maintained by those doing the coding actually wrong? No.

Now if you had substantial improvements you wanted to submit at the same time, that's different. Then you become one of the team, and you've earned a place at the table to argue that the style should be X. The rest of the group still may not agree with you, but at least then the group will likely listen as you've earned the right to ask.

The second most important point about coding style is consistency within a project. Without consistency it becomes more difficult to understand the code and detect errors within it. Of course the most important point is to make it easy to avoid introducing bugs, or to detect bugs that were inadvertently introduced into the program.

If your concerns about coding style doesn't address those two points, then you shouldn't be worried about it. If your concerns do address those two points, make sure you understand the people developing the program. Conventions that may be unclear to you may be perfectly clear to the developers.

If there's a legitimate bug in a certain block of code I'd argue you have a moral imperative to correct the more egregious coding style problems in that block while you're at it. Style for style's sake just introduces the possibility of introducing an error where there wasn't one before through unintended consequences.

First of all, coding style is important. Reading code is pattern matching and a concise style gives an easier reading experience.

Since coding style IS so important, just make it part of CI. Just like unit tests should be run on every change, so should style checking.

Ideally your language supports auto-formatting so you can't mess up the code with an automatic formatting. If you work in a whitespace-sensitive language it is a bit harder (auto-formatting can change semantics!) but in a braced language such as c#, java, c and so on, your editor should do this for you. Make sure the project has a file of expected style that can be used to configure the most likely editor(s).

I never understood why developers on ridiculously powerful computers think it's a sane idea manually editing for style and saving the changes back to the file. Couldn't we just have, oh I don't know, the computer change the display of the code based on that person's tastes? Is this because so many of us are still using tools made in the 70s?

80 chars has to die. Its fine for commit comments, but I'm tired of code running in a strip down 1/3 of my screen. Or are people coding with 20pt font nowadays?

Do you have a single editor window open at a time or something? I personally have many files open at once, and if the code runs down a third of my screen, that means I can split the screen in three horizontally, and thus have three files (or locations) visually available with a full screen height.

You can argue that 80 characters is too low for (insert language / project here). I agree that in many cases that's likely to be true. However, screen real estate is precious; if you have a window that has room for 150 characters of width and most of your lines are 40-80, you've got a ton of useless whitespace wasting perfectly good pixels.

It's a simple maximization problem. The character width should be wide enough to prevent too many lines of code from spilling over into two (or more) lines, but no more.

If there's a legitimate bug in a certain block of code I'd argue you have a moral imperative to correct the more egregious coding style problems in that block while you're at it. Style for style's sake just introduces the possibility of introducing an error where there wasn't one before through unintended consequences.

If you fix a bug in the same commit as style changes it makes it difficult to see why something changed. For example, if you had an off-by-one error (1 line change) that you also included style changes in and your change introduced a regression (e.g. in code depending on that off-by-one behaviour) it becomes difficult to tell why the bug occurred -- especially if indentation changes cause the diff to associate lines differently.

Also, if the file is in a different style to the rest of the code and you only change a single block, then that block becomes inconsistent with the coding style for the file.

Apple's recent security bug, "goto fail;", was caused by coding style. If they had had a "always use braces" rule the bug, embarrassment, expense and long hours would not have happened.

However, retrospectively converting to a new set of style rules into a large and mature code base can introduce bugs. If you have confidence in the project unit tests, if you are prepared to fix the bugs you will introduce and the rules you choose reduce risk more than risk of change then go for it.

Personally? Meh. After a while you realise coding style isn't worth the emotional investment you once gave it. Go for a walk instead.

80 chars has to die. Its fine for commit comments, but I'm tired of code running in a strip down 1/3 of my screen. Or are people coding with 20pt font nowadays?

Like much else in this discussion, I'd argue that it depends a lot on the project. I've worked on large codebases that you couldn't navigate without an IDE - 80 characters was irrelevant there: As long as it fit on your screen, it was fine.

I've also worked as a sysadmin on a system that had a lot of specialty perl/shell scripts, that you tended to need to read from the console or an xterm - 80 characters there was essential, as otherwise you'd be pulling your hair out down on the datafloor.

In all of this: What actual problem are you trying to solve, and is that problem relevant to this project? An automatic linting (or formatting) tool is great - if you can configure it for the project you are working on. Otherwise it's just a hoop you are making people jump through.

Add an issue/bug asking if there is interest in this kind of cleanup. Be specific about why it would benifit the project. Perhaps one of the warnings actually exposes a flaw.

I personally hate these. The tickets almost always boil down to "the style is different than what I use so it should be changed" or worse yet, micro-optimizations. They come up over and over again from newcomers to large projects and all they do is waste developer time.

The most important thing is for a project to be consistent with itself. Always defer to the project's style guide. If there isn't a style guide then it's okay to open a ticket asking for or volunteering to make one. (Though that can easily turn into a bikeshed problem if a project doesn't have a strong style to begin with.)

80 chars has to die. Its fine for commit comments, but I'm tired of code running in a strip down 1/3 of my screen. Or are people coding with 20pt font nowadays?

Do you have a single editor window open at a time or something? I personally have many files open at once, and if the code runs down a third of my screen, that means I can split the screen in three horizontally, and thus have three files (or locations) visually available with a full screen height.

You can argue that 80 characters is too low for (insert language / project here). I agree that in many cases that's likely to be true. However, screen real estate is precious; if you have a window that has room for 150 characters of width and most of your lines are 40-80, you've got a ton of useless whitespace wasting perfectly good pixels.

It's a simple maximization problem. The character width should be wide enough to prevent too many lines of code from spilling over into two (or more) lines, but no more.

No, I use tabs and dual screens. I understand many people don't have that (every developer I work with has at least two screens), but 80 chars is simply a holdover from an old generation. Most people can easily handle 120 chars with two side by side on a single screen. I agree there comes a point where you simply don't want the lines to be THAT long, but the 80 char limit is just too small for the "wide enough to prevent too many lines of code from spilling over", especially in object oriented languages where you have meaningfully named objects with meaningfully named methods.

According to Python's PEP-8 if the project has a default coding style that does not conform to the language's norms use the project's coding style not the language's. The whole point of a coding style is provide consistency in the code base so others can follow the it easily. Language default rules are useful because they provide basic, common default style and the Python PEP-8 acknowledges this point. I have read the Ruby default style guide.

80 chars has to die. Its fine for commit comments, but I'm tired of code running in a strip down 1/3 of my screen. Or are people coding with 20pt font nowadays?

Like much else in this discussion, I'd argue that it depends a lot on the project. I've worked on large codebases that you couldn't navigate without an IDE - 80 characters was irrelevant there: As long as it fit on your screen, it was fine.

I've also worked as a sysadmin on a system that had a lot of specialty perl/shell scripts, that you tended to need to read from the console or an xterm - 80 characters there was essential, as otherwise you'd be pulling your hair out down on the datafloor.

In all of this: What actual problem are you trying to solve, and is that problem relevant to this project? An automatic linting (or formatting) tool is great - if you can configure it for the project you are working on. Otherwise it's just a hoop you are making people jump through.

That's all fine, but the point was that someone was scouring projects on github and considered any line over 80 chars to be a bug. That mentality needs to die, coding with lines > 80 characters should not be taboo. There are certainly cases where you want to read/print on an 80 char terminal, but it should be OK for a project to choose to use a wider-lined style.

Apple's recent security bug, "goto fail;", was caused by coding style. If they had had a "always use braces" rule the bug, embarrassment, expense and long hours would not have happened.

Not necessarily true.My first thought was: it is an merge problem:

The diff looks like this:

+ goto fail;+ goto fail;

but the developer sees that

- goto fail:+ goto fail;

and assumes some minor change like whitespace.

Yes but with a style that required braces that double goto would not have mattered. Also the compiler should have warned about unreachable code was that a compiler bug or was that ignored. The best and most interesting disasters need a lot of thinks to go wrong at once.

No, I use tabs and dual screens. I understand many people don't have that (every developer I work with has at least two screens), but 80 chars is simply a holdover from an old generation. Most people can easily handle 120 chars with two side by side on a single screen. I agree there comes a point where you simply don't want the lines to be THAT long, but the 80 char limit is just too small for the "wide enough to prevent too many lines of code from spilling over", especially in object oriented languages where you have meaningfully named objects with meaningfully named methods.

Then I think we basically agree. As I said, I'm not specifically claiming an 80-character limit is correct for all projects. I could see 100 being good in modern code with more descriptive variable names and templates, maybe even more (though as DStaal points out many scripts and similar things are likely to be edited through an xterm). The way you phrased it, I initially thought you wanted to use the entire width of your desktop for a single file, and I thought that sounded wasteful in terms of horizontal space. I may have 8M pixels or so of desktop space but still want to use it all effectively. :-)

The style guide adopted by rubocop (written by the same author) is not definitive and actually conflicts on some points with GitHub's style guide and the practices of ruby core developers. For instance, rubocop will warn over the use of double quotations while many, including GitHub, advocate their use. Don't put too much faith in automated code analysis.

The real question is whether it's readable; if the coding style puts barriers between you and understanding what the code does, then yeah that code may need to be tidied up. But it's not such a simple issue as whether it's the coding style that needs to change, as better comments, more descriptive variable names and so-on could be more useful solutions as well.

Also, some of the "issues" that rubocop is flagging up aren't necessarily huge issues. While personally I stick to 80 characters per line even though I have plenty of space to go wider, I do it because any line that is getting longer than 80 characters is probably in dire need of a bit of whitespace to make it more readable anyway, either that or it's simply doing too much in a single line to be easy to understand. So it's still a good target to aim for IMO.Using curly braces to create distinct multi-line blocks is a trickier one; obviously in strictly scoped languages these are a great way to restrict the lifetime of a variable, but those aren't the only advantages of using blocks without conditionals. I like to use them because many IDEs let you collapse them, and they're generally a well-supported construct for dividing code into distinct segments, even if they're actually all acting within the same scope.Sensible use of single or double quotes is probably the one I'd be most concerned about, as it's far too easy to do things by accident because you've used the wrong type of quotes (in languages that distinguish them of course). As to whether you should go back and retroactively change all of the incorrect ones though is another matter; I definitely prefer developers to try and use the correct quotes for the situation, but they should probably only be "fixed" on a small scale, or if something is actually broken.

I found v2 much easier to understand. Shrug. That's old age for you. YMMV.

The point here is indentation of expressions and functional-programming style code. Most C++ or C style checkers do not expect nice formating of expressions (functional-style code). If you ever programmed a functional programming language, you know formatting expressions is as important as formating stuctured programming in C. For the same reason.

I found v2 much easier to understand. Shrug. That's old age for you. YMMV.

The point here is indentation of expressions and functional-programming style code. Most C++ or C style checkers do not expect nice formating of expressions (functional-style code). If you ever programmed a functional programming language, you know formatting expressions is as important as formating stuctured programming in C. For the same reason.

That looks more like someone trying to imitate a beginners LISP code and the autoformatter was either written by an idiot or configured by one (logical operator at the *start* of a line? no consideration for parens? yeah only logical explanation)

Writing the version you would expect to see in C++ (still some unnecessary braces, but it's often a good idea to make the precedence clear) without any newlines thrown in, we get something like:

Which is pretty much exactly how I'd format it manually - a far cry from unreadable in any case (adding the strange parens back in btw results in the same layout just with the useless parens around the comparisons).

That looks more like someone trying to imitate a beginners LISP code and the autoformatter was either written by an idiot or configured by one (logical operator at the *start* of a line? no consideration for parens? yeah only logical explanation)

Writing the version you would expect to see in C++ (still some unnecessary braces, but it's often a good idea to make the precedence clear) without any newlines thrown in, we get something like:

Which is pretty much exactly how I'd format it manually - a far cry from unreadable in any case (adding the strange parens back in btw results in the same layout just with the useless parens around the comparisons).

I would format that code just like you do but I do not find the lisp style format bad at all it is readable it looks OK it is compact. What do you think are the disadvantages of lisp style formatting?

That looks more like someone trying to imitate a beginners LISP code and the autoformatter was either written by an idiot or configured by one (logical operator at the *start* of a line? no consideration for parens? yeah only logical explanation)

Writing the version you would expect to see in C++ (still some unnecessary braces, but it's often a good idea to make the precedence clear) without any newlines thrown in, we get something like:

Which is pretty much exactly how I'd format it manually - a far cry from unreadable in any case (adding the strange parens back in btw results in the same layout just with the useless parens around the comparisons).

I would format that code just like you do but I do not find the lisp style format bad at all it is readable it looks OK it is compact. What do you think are the disadvantages of lisp style formatting?

What I find amusing in all of these debates is just how many different ways there are to format the same code. I would format it as:

I would format that code just like you do but I do not find the lisp style format bad at all it is readable it looks OK it is compact. What do you think are the disadvantages of lisp style formatting?

It's not actually "lisp style" - if you pretty print the corresponding LISP code you'll arrive at a completely different result! In lisp you *never* put each closing parens on its own line and keep a structure like you'd do in C code with brackets - that's why I say "beginner's lisp code", it's something that's ingrained in people coming from C-like languages and takes a while to get rid of. As someone who knows LISP I'm probably way more sensitive to this than others.

Anyhow it's not a question of whether it's better or worse - it's just not idiomatic C++ code. You can break such assumptions/idioms if you have a good reason, but there's just no advantage here to do so.

I find breaking the line without ending in an operand rather unreadable because you break the logical ordering. It's also contrary to how it's done in math publications or any style guide I'm familiar with (PEP8, google c++, Sun's Java guide, MS' C#,..).

But really the most important thing is to keep the indentation logically sensible (i.e. you want to indent the start of a subexpression to be one level deeper than the higher level expression), if you do that the rest is really not a big deal. That's actually the biggest problem of the first "auto formatter" and what makes that one really unreadable compared to v1 "lisp". And as we see the human proposed versions all do to some degree or other.

If you send me a pull request and could have done it with less changes to the existing code base, I will reject it.

It has nothing to do with whether I think your code style is worse or better than mine, I just don't want you to waste my time by making me check if your updated code still works. So I'm simply going to knock it back.

Which is pretty much exactly how I'd format it manually - a far cry from unreadable in any case (adding the strange parens back in btw results in the same layout just with the useless parens around the comparisons).

They're both totally unreadable, it took me 5 minutes to even figure out what the code actually does.

Once I figured that out, here's how I'd have written it:

Code:

if (!valid) return;

if (val1 != longconstant || val2 != anotherlongconstant) return;

if (val1 != andjustforfunonemorelongconstant) return;

And there we go, longest line is about 60 characters and I didn't fake it by inserting newlines in the middle of a statement.

It's not the length of the line that matters, it's the length of the statement. A 120 character statement is 120 characters regardless of how many lines it spans.

But if someone sent that change as a pull request, I'd reject it. Unless the change required changes inside the if statement.