Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: build, test
    • Labels:
      None

      Description

      Our coding guidelines say that hard tabs are disallowed in the Hadoop code, but they sometimes sneak in (there are about 280 in the common codebase at the moment).

      We should run a simple check for this in the test-patch process so it's harder for them to sneak in.

      1. hadoop-7334.txt
        1 kB
        Todd Lipcon
      2. hadoop-7334.txt
        1 kB
        Todd Lipcon

        Activity

        Hide
        Todd Lipcon added a comment -

        I tested this on a patch that had some tabs, and it correctly identified them.

        Show
        Todd Lipcon added a comment - I tested this on a patch that had some tabs, and it correctly identified them.
        Hide
        Nigel Daley added a comment -

        +1. Looks like it only checks for tabs on + lines.

        Show
        Nigel Daley added a comment - +1. Looks like it only checks for tabs on + lines.
        Hide
        Jonathan Eagles added a comment -

        Todd, I wondering if we should remove the '-c' on the first grep. I'm in favor of coding style checking. Do you think we have a need to do something like checkstyle in the future?

        Show
        Jonathan Eagles added a comment - Todd, I wondering if we should remove the '-c' on the first grep. I'm in favor of coding style checking. Do you think we have a need to do something like checkstyle in the future?
        Hide
        Jonathan Eagles added a comment -

        Other than what I said above, I think this patch looks great.

        Show
        Jonathan Eagles added a comment - Other than what I said above, I think this patch looks great.
        Hide
        Todd Lipcon added a comment -

        You're absolutely right about the first -c.. I was testing with a patch that had only one tab so I didn't notice

        Something like checkstyle probably makes sense at some point, but right now we have so many violations that it would be a big change. Spaces vs tabs is one thing everyone agrees on and where there's never any question of "aesthetics" So let's start here and maybe some day ease in some checkstyle.

        Show
        Todd Lipcon added a comment - You're absolutely right about the first -c.. I was testing with a patch that had only one tab so I didn't notice Something like checkstyle probably makes sense at some point, but right now we have so many violations that it would be a big change. Spaces vs tabs is one thing everyone agrees on and where there's never any question of "aesthetics" So let's start here and maybe some day ease in some checkstyle.
        Hide
        Tsz Wo Nicholas Sze added a comment -

        Nice work! We should have this long time ago.

        BTW, do you want to also check 80 characters per line? But it is arguably if it is the rule we want to enforce.

        Show
        Tsz Wo Nicholas Sze added a comment - Nice work! We should have this long time ago. BTW, do you want to also check 80 characters per line? But it is arguably if it is the rule we want to enforce.
        Hide
        Todd Lipcon added a comment -

        IMO 80 chars per line is one of those things that should be kept in general, but if the occasional line flows over it's not a big deal. There's some times when it's much less readable to try to keep to it.

        But this is where we get into religious wars I'm a 100-per-line devotee myself!

        Show
        Todd Lipcon added a comment - IMO 80 chars per line is one of those things that should be kept in general, but if the occasional line flows over it's not a big deal. There's some times when it's much less readable to try to keep to it. But this is where we get into religious wars I'm a 100-per-line devotee myself!
        Hide
        Tsz Wo Nicholas Sze added a comment -

        Provided that most monitors are wide-screen nowadays, the 80-per-line rule is probably out-dated.

        Show
        Tsz Wo Nicholas Sze added a comment - Provided that most monitors are wide-screen nowadays, the 80-per-line rule is probably out-dated.
        Hide
        Tom White added a comment -

        > Do you think we have a need to do something like checkstyle in the future?

        We have long had a checkstyle target in ant, and plots of checkstyle violations over time (see https://builds.apache.org/job/Hadoop-Common-trunk/), but unfortunately we haven't enforced the rule that the number may not increase (unlike javadoc or findbugs warnings for example).

        Another way of implementing this JIRA would be to enable such a rule (perhaps with a weaker set of checkstyle rules than the current set, e.g. drop the 80-per-line rule).

        Show
        Tom White added a comment - > Do you think we have a need to do something like checkstyle in the future? We have long had a checkstyle target in ant, and plots of checkstyle violations over time (see https://builds.apache.org/job/Hadoop-Common-trunk/ ), but unfortunately we haven't enforced the rule that the number may not increase (unlike javadoc or findbugs warnings for example). Another way of implementing this JIRA would be to enable such a rule (perhaps with a weaker set of checkstyle rules than the current set, e.g. drop the 80-per-line rule).
        Hide
        Nigel Daley added a comment -

        Tom, that's a good idea. Enable checkstyle with just the tab check and then we can grow the checks over time. Start small and iterate.

        Show
        Nigel Daley added a comment - Tom, that's a good idea. Enable checkstyle with just the tab check and then we can grow the checks over time. Start small and iterate.

          People

          • Assignee:
            Todd Lipcon
            Reporter:
            Todd Lipcon
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:

              Development