Is it okay to make coding style changes on an open source project that doesn't follow best practices?

Question

Recently, I came across a number of open source Ruby (or majority of it was Ruby) projects on GitHub that when checked with a code analyzing tool like Rubocop, create a lot of offenses.

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.

[The] Ruby style guide recommends best practices so that real-world Ruby programmers can write code that can be maintained by other real-world Ruby programmers. ~ Source: Ruby Style Guide

Although they are small and easy to fix, is it appropriate to change the coding style of an open source project by fixing the offenses 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 offenses 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?

[The author(s) of the Ruby Style Guide] didn't come up with all the rules out of nowhere - they are mostly based on my extensive career as a professional software engineer, feedback and suggestions from members of the Ruby community and various highly regarded Ruby programming resources, such as "Programming Ruby 1.9" and "The Ruby Programming Language". ~ Source: Ruby Style Guide

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

Answer 1

Ask the maintainers.

Coding style is a quite subjective discussion, and rules like maximum line length of 80 characters are fairly subjective - 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.

Answer 2

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 "Talk is cheap, show me the code".

Answer 3

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 a wonder how often junior types confuse opinion for fact.

Answer 4

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 offering 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 issues, 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 cause havoc with many IDEs. I can think of three largely popular projects that use deprecated flags incorrectly that 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.

Answer 5

Taken from the Rubocop site itself (emphasis mine):

One thing has always bothered me as a Ruby developer - Python developers have a great programming style reference (PEP-8) and we never got an official guide, documenting Ruby coding style and best practices.

Please understand:

There is no Official Ruby Style Guide

I'm not saying that style guides are bad. But not only is there no official guide, but having a style guide is a choice that is made on a personal, project, team, company level. Style guides are, to use a psychology term, an "in-group social norm".

What does that mean for you? Well, if you aren't considered part of the in-group, that means anything you - or any other website says - is highly unlikely to hold any sway with the group. So if you aren't an active, respected contributor to that specific project then most likely your suggestions will be ignored, or will at best be a reminder of previous considerations about having a style guide. At worst it will be taken as an insult, or as an interloper or bikeshedder sticking their nose in where it doesn't belong.

Can't you just suggest a Style Guide?

In fact, that seems to be what you want to do: you believe in the value of style guides, you highly value consistency, and you want to evangelize for the dedication to unified style guidelines.

That's fine, so long as you are really clear that's what you are about and what you want to accomplish. If you believe in a particular Style Guide and believe it is The One True Style Guide, or at least better than whatever those lawless heathens are running around practicing, then that's fine too.

But what people do not appreciate is being told that their behavior does not conform to unofficial, non-binding, largely arbitrary rules that are from a source they do not consider a legitimate authority. When that's what you set out to do, or if merely that is what you are perceived to be doing, you'll get less of "the red carpet treatment", and more of the "angry natives with spears and a big boiling pot" treatment.

Answer 6

To offer an alternate opinion here, in many cases, such a change would be welcome. Open source projects tend to have many authors. There is often no "coding style"; the style is just whatever the person who wrote the code in question happened to use. If one file was written by a different person than another, the style may be different as well. Even in projects where there is a consensus style, unless they check for this style regularly, it often is not used.

A common example of this in my experience is when someone contributes some code via pull request that is of relatively low quality style-wise. However, the code may work. Different people have different views on this. Some people will refuse to merge a pull request unless the style is good. Some people don't care, as long as the code works. Some people prefer to have good style, but they don't want to scare off contributors with a bunch of "fix the whitespace here" comments (I personally always feel a little guilty making these comments, even though I know they are for the better good of the codebase, because it it does feel like it might scare the contributor away).

So don't assume straight out that the style that you see is the style that the project wants. In fact, that can probably be generalized to contributing to open source in general: don't assume that the code an open source project has is the code that it wants.

You should be aware of some things, though:

• Some people are religious about style. If it becomes clear that they don't want to budge, then don't bother.

• This is a huge bikeshed issue. Everybody and their brother has an opinion about these things. Getting such a pull request merged can be difficult because of this.

• It can also be hard to get such a pull request merged because it will get merge conflicts very quickly; basically whenever any part of the codebase that you modified changes, even if its in trivial ways.

I would stick with the "ask first" approach. If they are open to it, and you are willing to stick with the pull request to completion, then go for it.

Answer 7

Although they are small and easy to fix, do you think it is okay to change the coding style of an open source project by fixing the offences and making a Pull Request?

In short, no!

Of course, I'm assuming here that these are just matters of style and aren't actual bugs - pull requests for the latter would always be sensible (IMO.) I'm also assuming that you're a stranger to the project and not in contact with the people who maintain it already.

However, amongst any language there's always some people who have their preferences which differ from the style guide, for better or worse, and trying to enforce those style changes on people that you don't know, and a project that you're not part of if nothing else could comes across as a bit rude. After all - what are you achieving (in reality) if the request was accepted? In all likelihood, if the members of one project wanted to change the style to something different, they would have done it already - and all you'd be doing with that request is forcing a style on them that doesn't necessarily work any better for the existing members.

This changes slightly if you're willing to contribute to the project in other ways other than doing style "fixes". I'd say if you want to open a dialogue with the maintainers about how you'd like to work on the project but find a non-standard style difficult, then that's fine. But I really wouldn't just go around blindly creating pull requests for a bunch of projects that only contain style changes!

Answer 8

ANY change is liable to introduce bugs, even changing typographical formating of the code.
Therefore no changes should be performed on the code unless there's a valid business case, and "but the code doesn't look nice" or "the code doesn't follow the coding standards" is not a valid business case. The risk is simply too great.
Now, if you're making major changes to a source file anyway, pulling the entire file in line with the standards might be acceptable, but most likely you will be making small changes in which case it's almost universally preferable to keep your own changes in line with the existing code even though that existing code does not follow the coding standards.
Heck, the code could well be the result of a code generator and at times be regenerated. Code generators are notorious for producing ugly code...

Answer 9

I used to be big on having a good style guide, but given the state of affairs in Ruby, "I've moved on".

Basically I live with what I'm working with and otherwise follow the general conventions I've learned from a number of jobs.

For Ruby, which is my language of choice I've also (in my head) divided style into universally accepted, generally accepted, my preferences and best practices. Things that are universally accepted I might submit a change as part of a change request for an issue or feature branch request.

Examples of each style (in my opinion):

Universally accepted for Ruby:

• spaces not tabs
• two space indents
• method_names_use_underscores
• Constants start with Caps.

Generally accepted:

• Don't use parentheses if not necessary
• Use { } for one line blocks and do end for multi-line blocks.
• CONSTANTS_ARE_IN_ALL_CAPS
• Use predicative statements (cond ? true : false) over if then if the expression fits on 1 line.

Personal preferences:

• Line length of 120 character lines
• case when statements are indented 2 from the case statement.
• Use single quotes unless double are needed.

No Agreement:

• Case Statements
• Line length

Best Practices:

• small methods, <= 5 lines if possible.
• small classes, <= 100 lines if possible.
• Avoid constants
• Code has tests

Finally, as others have detailed, you should ask first. At the end of the day the key to style is communication between developers and sensitivty to others. For example if I want to make a style change on an open source project, I will often do a pull request for one or two actual features or bug fixes first. Once the maintainer knows me and sees that I've contributed, then I might suggest style changes. I only suggest them though. For example "While doing another feature on project x, I noticed that you have a 4 space indents in a couple of files and I was wondering if I could change them to 2 ?"

Answer 10

I have a couple of moderately successful .NET projects, and I have had a couple of PRs from people who appear to have gone through the code with ReSharper and StyleCop and "fixed" a bunch of stuff. I don't accept those PRs, for a couple of reasons:

• While many of the changes are for the better, some of them negatively impact the code in some way, usually performance, and cherry-picking the good parts is too much hard work.
• Even if all the changes were either benign or even beneficial, every change in every file in every commit needs to be reviewed, and again, it just takes too long.

That said, if anyone wanted to add some better error checking or doc comments, I'd accept that PR in a heartbeat.

Answer 11

You need to find out if the maintainers consider these coding style violations a defect, or if the project has a different standard for coding style.

If it has a different standard, you could offer to help document or formalize it. Then it might turn out there are some violations of that style, and you could fix those.