Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.7.0
    • Fix Version/s: 0.8.0
    • Component/s: general
    • Labels:
      None

      Description

      We need to expand https://cwiki.apache.org/confluence/display/BIGTOP/How+to+Contribute with

      • formatting guidelines for groovy . groovy seems tricky to format, not sure what guidelines to follow.
      • Also bigtop has many other file types (bash, puppet, etc...) so probably good to outline expectations for those to, or at least link to references.
      • patch submission guidelines : if we want to use reviewboard or other review tools how we will use them, and in what cases.

      This will make minor nits easier : just reference the code formatting guidelines.

      1. ssht.jpg
        274 kB
        jay vyas

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          185d 9h 35m 1 Konstantin Boudnik 06/Sep/14 07:58
          Resolved Resolved Closed Closed
          7h 7m 1 jay vyas 06/Sep/14 15:06
          jay vyas made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Hide
          jay vyas added a comment -

          we can close it. For now, got to https://cwiki.apache.org/confluence/display/BIGTOP/How+to+Contribute#HowtoContribute-REVIEWINGPATCHES To review, update, and refresh guidelines.

          Show
          jay vyas added a comment - we can close it. For now, got to https://cwiki.apache.org/confluence/display/BIGTOP/How+to+Contribute#HowtoContribute-REVIEWINGPATCHES To review, update, and refresh guidelines.
          Konstantin Boudnik made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Konstantin Boudnik added a comment -

          Fixed in the wiki

          Show
          Konstantin Boudnik added a comment - Fixed in the wiki
          Konstantin Boudnik made changes -
          Assignee jay vyas [ jayunit100 ]
          Hide
          Konstantin Boudnik added a comment -

          On a separate note: jay vyas are we still working on this ticket or it can be closed? Seems like latter, as we now have the wiki page to keep the effort going. no?

          Show
          Konstantin Boudnik added a comment - On a separate note: jay vyas are we still working on this ticket or it can be closed? Seems like latter, as we now have the wiki page to keep the effort going. no?
          Hide
          Konstantin Boudnik added a comment -

          I've fixed it. Thanks.

          Show
          Konstantin Boudnik added a comment - I've fixed it. Thanks.
          Hide
          Martin Bukatovic added a comment -

          Maybe a stupid note, but I noticed that mentioned page refers to Sun coding conventions, but the link is broken. The document has been moved to: http://www.oracle.com/technetwork/java/codeconvtoc-136057.html

          Show
          Martin Bukatovic added a comment - Maybe a stupid note, but I noticed that mentioned page refers to Sun coding conventions, but the link is broken. The document has been moved to: http://www.oracle.com/technetwork/java/codeconvtoc-136057.html
          jay vyas made changes -
          Link This issue relates to BIGTOP-1286 [ BIGTOP-1286 ]
          Konstantin Boudnik made changes -
          Fix Version/s 0.8.0 [ 12324841 ]
          Konstantin Boudnik made changes -
          Affects Version/s 0.7.0 [ 12324362 ]
          Hide
          jay vyas added a comment -

          just a note when we finally make this page to take the last couple of review in BIGTOP-952 and add those comments into this as well.

          • when to use groovy "def" vs "final" ...
          • idiomatic use groovy primitives for easy null checking/maps/lists creation
          • use safe groovy casting (as List instead of (List))
          • idiomatic JSON processing
          • how to write a shebang
          • Using Commons logger (or slf4j maybe?) for all print statements
          Show
          jay vyas added a comment - just a note when we finally make this page to take the last couple of review in BIGTOP-952 and add those comments into this as well. when to use groovy "def" vs "final" ... idiomatic use groovy primitives for easy null checking/maps/lists creation use safe groovy casting (as List instead of (List)) idiomatic JSON processing how to write a shebang Using Commons logger (or slf4j maybe?) for all print statements
          Hide
          jay vyas added a comment -
          Show
          jay vyas added a comment - okay done https://issues.apache.org/jira/browse/INFRA-7406
          Hide
          Konstantin Boudnik added a comment -

          hmm... once you've mentioned it - I don't have the edit link too. I think you need to open a ticket with INFRA guys. I think Peter Linnell and others had these problems earlier.

          Show
          Konstantin Boudnik added a comment - hmm... once you've mentioned it - I don't have the edit link too. I think you need to open a ticket with INFRA guys. I think Peter Linnell and others had these problems earlier.
          jay vyas made changes -
          Field Original Value New Value
          Attachment ssht.jpg [ 12632910 ]
          Hide
          jay vyas added a comment -

          hmm...
          screenshot attached. where do i click to edit this thing?

          Show
          jay vyas added a comment - hmm... screenshot attached. where do i click to edit this thing?
          Hide
          Konstantin Boudnik added a comment -

          I have shared the page with you, but it actually doesn't have any edit-restrictions.

          Show
          Konstantin Boudnik added a comment - I have shared the page with you, but it actually doesn't have any edit-restrictions.
          Hide
          jay vyas added a comment -

          Yeah sure

          and yes, dont worry i wont propogate my funny patch names into the official wiki

          PS i dont think i have wiki perms. can you set them for me? thanks

          Show
          jay vyas added a comment - Yeah sure and yes, dont worry i wont propogate my funny patch names into the official wiki PS i dont think i have wiki perms. can you set them for me? thanks
          Hide
          Konstantin Boudnik added a comment -

          Jay, it makes sense - thank you! One comment:

          set the "strip trailing spaces" option on save for all files

          It should be set to "Save for modified files" - otherwise it will produce a storm of unwarranted white-space changes in every patch.

          run "git am ../bigtop-jayunit100/BIGTOP-1221.4.intellij.patch

          I won't encourage these custom naming schema for the patches, but it might be just me ?

          Can you add this to the wiki?

          Show
          Konstantin Boudnik added a comment - Jay, it makes sense - thank you! One comment: set the "strip trailing spaces" option on save for all files It should be set to "Save for modified files" - otherwise it will produce a storm of unwarranted white-space changes in every patch. run "git am ../bigtop-jayunit100/ BIGTOP-1221 .4.intellij.patch I won't encourage these custom naming schema for the patches, but it might be just me ? Can you add this to the wiki?
          Hide
          Bruno Mahé added a comment -

          It would be great to export and checkin an Eclipse code style formatter at least for java. This would make it dead easy to have compliant formatting.
          WRT groovy I have no idea since I don't use groovy.

          Show
          Bruno Mahé added a comment - It would be great to export and checkin an Eclipse code style formatter at least for java. This would make it dead easy to have compliant formatting. WRT groovy I have no idea since I don't use groovy.
          Hide
          jay vyas added a comment - - edited

          FYI, here is what ive done to make it easier to create bigtop compliant patches. I think this should/could go somewhere so its easy to add new patches but maybe its obvious to everyone but me

          • use intellij's groovy editor. It appears to default to spaces (instead of tabs).
          • set the "strip trailing spaces" option on save for all files
          • set "Code style -> General -> Right margin" to 80
          • set "Code style -> Groovy ->" to tabsize=4, indent=2, continuation_indent=4
          • checked the "Code style -> Groovy " : "Ensure right margin is not exceeded" box.
          • reload all files at this point, so that settings take.
          • Then use git to commit the patch in a single go, so that whole commit is together.
          • Then use mark grovers trick to create patches:
          • git format-patch HEAD^..HEAD --stdout > BIGTOP-xxxx.patch
          • Then to validate the patch:
          • run "git am ../bigtop-jayunit100/BIGTOP-1221.4.intellij.patch " on a clean bigtop head, and make sure you get now "whitespace" errors.
          Show
          jay vyas added a comment - - edited FYI, here is what ive done to make it easier to create bigtop compliant patches. I think this should/could go somewhere so its easy to add new patches but maybe its obvious to everyone but me use intellij's groovy editor. It appears to default to spaces (instead of tabs). set the "strip trailing spaces" option on save for all files set "Code style -> General -> Right margin" to 80 set "Code style -> Groovy ->" to tabsize=4, indent=2, continuation_indent=4 checked the "Code style -> Groovy " : "Ensure right margin is not exceeded" box. reload all files at this point, so that settings take. Then use git to commit the patch in a single go, so that whole commit is together. Then use mark grovers trick to create patches: git format-patch HEAD^..HEAD --stdout > BIGTOP-xxxx.patch Then to validate the patch: run "git am ../bigtop-jayunit100/ BIGTOP-1221 .4.intellij.patch " on a clean bigtop head, and make sure you get now "whitespace" errors.
          Hide
          Konstantin Boudnik added a comment - - edited

          Actually, we are following Hadoop coding guidelines - I am pretty sure it has been stated elsewhere.
          There's nothing wrong with groovy formatting - they are the same as Java's. You just need to use right tools for it. And Eclipse isn't a right tool.... for anything

          Reviewboard can be used as a second-line tool: patches have to be attached to JIRA and reviews need to be posted as JIRA comments. If this requires an extra integration with reviewboard - some one needs to address it.

          Show
          Konstantin Boudnik added a comment - - edited Actually, we are following Hadoop coding guidelines - I am pretty sure it has been stated elsewhere. There's nothing wrong with groovy formatting - they are the same as Java's. You just need to use right tools for it. And Eclipse isn't a right tool.... for anything Reviewboard can be used as a second-line tool: patches have to be attached to JIRA and reviews need to be posted as JIRA comments. If this requires an extra integration with reviewboard - some one needs to address it.
          jay vyas created issue -

            People

            • Assignee:
              jay vyas
              Reporter:
              jay vyas
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development