Details

    • Type: Test Test
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Implemented
    • Affects Version/s: None
    • Fix Version/s: HADOOP-12111
    • 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

        Issue Links

          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.
          Hide
          Allen Wittenauer added a comment -

          tab support now in yetus. closing as implemented.

          Show
          Allen Wittenauer added a comment - tab support now in yetus. closing as implemented.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development