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

          Todd Lipcon created issue -
          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.
          Todd Lipcon made changes -
          Field Original Value New Value
          Attachment hadoop-7334.txt [ 12480570 ]
          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.
          Todd Lipcon made changes -
          Attachment hadoop-7334.txt [ 12480618 ]
          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.
          Allen Wittenauer made changes -
          Issue Type Improvement [ 4 ] Test [ 6 ]
          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.
          Allen Wittenauer made changes -
          Link This issue Is contained by HADOOP-12113 [ HADOOP-12113 ]
          Allen Wittenauer made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s HADOOP-12111 [ 12332834 ]
          Resolution Implemented [ 10 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          1492d 23h 2m 1 Allen Wittenauer 27/Jun/15 18:46

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development