Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
3  
Similar question: Should I refactor a F class from code climate?, but with more focus on architecture. –  amon Feb 2 at 14:22
15  
5  
Many of these style issues, sound pretty minor tbh. –  JL235 Feb 2 at 16:23
5  
3  
@MathewFoscarini no, what they're asking is, "if I buy a radar gun, can I decide what the local driving laws are?" –  Jon Hanna Feb 3 at 2:23

11 Answers 11

up vote 62 down vote accepted

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.

share|improve this answer
31  
Many maintainers also don't like large changes that are not strictly necessary because they tend to create merge conflicts that are not strictly necessary either. –  Jan Hudec Feb 3 at 9:57
3  
You will lost the annotate function (who create the line of code) in source control, if there is a bug it is hard to blame where it was introduced and track if there are more of that sort of error. –  peer Feb 3 at 14:55
3  
In addition - if the project specific conventions are consistent, you could create a project specific configuration for the convention checking tool and offer the conventions config as pull request. So the maintainers can check their project using their configs. (I do not know if this is possible with the tool you used.) –  Peter Kofler Feb 3 at 17:20
    
+1 on the merge conflicts. I just accepted a coding style rework and I wish I hadn't, since it's caused merge conflicts with other people's PRs. It's easy to resolve but I'd have rather done it piece by piece –  Daenyth Feb 28 at 23:37

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".

share|improve this answer
1  
Best answer. You must respect the efforts of those who came before when submitting pull requests. Changing the project style underneath the feet of all existing developers, especially everyone who has higher 'rank' than you in the meritocracy, makes it difficult for them to remember the new rules you introduce. This would hurt their ability to maintain the project the way they have been. Furthermore if you were active in the project's development already, you'd probably know the maintainer's thoughts on stylistic changes and the best point in the project's lifecycle to introduce them. –  Chris Keele Feb 4 at 14:32
1  
Exactly. For example the Linux project, despite having a quite strict style guide for new pieces of code, will not allow you to simply submit a big pull request fixing styling issues, because it buggers with merging. It even asks you to maintain the local style, even if that means that it goes against the project style guide. –  Miles Rout Feb 4 at 23:24

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.

share|improve this answer
9  
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. –  Jan Hudec Feb 3 at 9:41

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.

share|improve this answer
    
In your opinion, would you consider the benefits for future contributors a valid "excuse" for a Pull Request fixing offences? For example, so that future contributors don't need to worry about looking through the whole code base in order to grasp its coding style. –  Rafal Chmiel Feb 2 at 15:06
    
@RafalChmiel When an author accepts a pull request, the person who sent that pull gets added to the repo and publicly listed as a contributor to the project and they stay listed as a contributor. When reviewing a pull request the author will keep this in mind when deciding to accept it. Keep that in mind when sending pull requests. Do stuff worthy of being a contributor. –  Mathew Foscarini Feb 2 at 15:23
    
What I meant was would benefiting future contributors by fixing offences be a valid reason for merging a PR with such fixes? Why should the maintainer merge such PR? Maybe the wording of my question was a bit off. –  Rafal Chmiel Feb 2 at 15:49
2  
@RafalChmiel everything you state as a reason is just your opinion. If it's offensive code that looks like bull, then write your fears in an issue. If the author defends against such a pull, then wipe your tears with a tissue. –  Mathew Foscarini Feb 2 at 16:05
    
Great answer, thanks a lot! –  Rafal Chmiel Feb 2 at 16:06

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.

share|improve this answer

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.

share|improve this answer

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 ?"

share|improve this answer

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!

share|improve this answer

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...

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer
    
this does not seem to offer anything valuable over another answer that was posted several hours prior to this one –  gnat Feb 3 at 11:09

protected by GlenH7 Feb 28 at 21:10

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.