Add hooks module, containing utility functions common to hook scripts.

Review Request #5543 — Created Feb. 24, 2014 and submitted

Information

RBTools
master
0c6bd5c...

Reviewers

This is a new 'hooks' module in RBTools. rbtools/hooks/common.py contains functions common to all hook scripts, and rbtools/hooks/git.py contains functions for Git hook scripts.

  • Tested the functions individually, and got the expected results.
  • More testing was done in https://reviews.reviewboard.org/r/5403/ for the Git post-receive and pre-receive hook scripts that use this new module.
Description From Last Updated

There should be two blank lines between items at the lowest indentation level. Here and below.

daviddavid

This should probably propagate up the message in the exception.

daviddavid

This should probably check that the command was sucessful.

daviddavid

Should it be up to the caller to pass in a compiled regex with the options that they care to …

daviddavid

This can just return directly instead of assigning to a variable and returning that.

daviddavid

Two blank lines between top-level blocks. Here and below.

daviddavid

It might be nice to pass in arguments as a list instead of relying on shell=True to split it.

daviddavid

Relying on the pipe here makes it so this only works on unix-type systems (and even then relies a lot …

daviddavid

You could use '0' * 40 here.

daviddavid

No need for this line.

daviddavid

No need for this line.

daviddavid

"URL"

chipx86chipx86

So I believe that Christian's comment about _make_api_client was that it's only used in this one place, so instead of …

daviddavid
david
  1. 
      
  2. rbtools/hooks/common.py (Diff revision 1)
     
     
    Show all issues

    There should be two blank lines between items at the lowest indentation level. Here and below.

  3. rbtools/hooks/common.py (Diff revision 1)
     
     
     
    Show all issues

    This should probably propagate up the message in the exception.

  4. rbtools/hooks/common.py (Diff revision 1)
     
     
     
    Show all issues

    This should probably check that the command was sucessful.

  5. rbtools/hooks/common.py (Diff revision 1)
     
     
    Show all issues

    Should it be up to the caller to pass in a compiled regex with the options that they care to use?

  6. rbtools/hooks/common.py (Diff revision 1)
     
     
     
    Show all issues

    This can just return directly instead of assigning to a variable and returning that.

  7. rbtools/hooks/git.py (Diff revision 1)
     
     
    Show all issues

    Two blank lines between top-level blocks. Here and below.

  8. rbtools/hooks/git.py (Diff revision 1)
     
     
     
    Show all issues

    It might be nice to pass in arguments as a list instead of relying on shell=True to split it.

  9. rbtools/hooks/git.py (Diff revision 1)
     
     
     
    Show all issues

    Relying on the pipe here makes it so this only works on unix-type systems (and even then relies a lot on shell=True parsing this). You could do this filtering pretty easily in python instead.

  10. rbtools/hooks/git.py (Diff revision 1)
     
     
    Show all issues

    You could use '0' * 40 here.

  11. rbtools/hooks/git.py (Diff revision 1)
     
     
    Show all issues

    No need for this line.

  12. rbtools/hooks/git.py (Diff revision 1)
     
     
    Show all issues

    No need for this line.

  13. 
      
anselina
chipx86
  1. Guess I never published this. Meant to :/

  2. rbtools/hooks/common.py (Diff revision 2)
     
     

    Just curious. Why add this function instead of just instantiating RBClient where needed?

    1. Hm, I actually copied this function and get_api() from rbtools/commands/init.py. Can this be improved?

  3. rbtools/hooks/common.py (Diff revision 2)
     
     
    Show all issues

    "URL"

  4. 
      
anselina
anselina
anselina
david
  1. 
      
  2. rbtools/hooks/common.py (Diff revision 5)
     
     
    Show all issues

    So I believe that Christian's comment about _make_api_client was that it's only used in this one place, so instead of wrapping it in a function you can just replace this line with:

    api_client = RBClient(server_url, username=username,
                          password=password)
    
  3. 
      
anselina
david
  1. This looks good to me. Are there any other changes you want to make before I push it?

    1. I gave this and /r/5403 a final round of testing, and found one last bug I must've missed while adapting the code for the pre-receive approval hook. In any case, the latest revision fixes it. I won't be making any further changes, so this is ready to be pushed. :)

  2. 
      
anselina
david
  1. Ship It!

  2. 
      
anselina
Review request changed
Status:
Completed
Change Summary:
Pushed to master (468379e). Thanks!