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

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

anselina
RBTools
master
5403
0c6bd5c...
rbtools, students

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)
     
     

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

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

    This should probably propagate up the message in the exception.

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

    This should probably check that the command was sucessful.

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

    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)
     
     
     

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

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

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

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

    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)
     
     
     

    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)
     
     

    You could use '0' * 40 here.

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

    No need for this line.

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

    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)
     
     
  4. 
      
anselina
anselina
anselina
david
  1. 
      
  2. rbtools/hooks/common.py (Diff revision 5)
     
     

    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: Closed (submitted)

Change Summary:

Pushed to master (468379e). Thanks!
Loading...