New rb-config command - shows user configuration.

Review Request #2548 — Created Aug. 14, 2011 and submitted

Information

RBTools

Reviewers

For now we can't provide command for changing '.reviewboardrc', so the new command only lists configuration values in pretty form.

 
Description From Last Updated

The "by default," wasn't addressed.

chipx86chipx86

No need for the \, since the parens guarantee Python won't get confused. "by default," (with the comma) and not …

chipx86chipx86

Seems like the set() math should be part of load_config_files instead. Once you've done that, you no longer need to …

chipx86chipx86

Can you import rbconfig instead, and then call rbconfig.main()?

chipx86chipx86

ouput?

chipx86chipx86

No need for re.split. You can just do str.split(delimiter). Also, we should avoid map and filter where possible. They're less …

chipx86chipx86

Should be clear that it's testing in rb config.

chipx86chipx86

Should be clear that it's testing in rb config.

chipx86chipx86

"Test the '-a/--all' option in rb-config"

chipx86chipx86

This needs to be: try: from cStringIO import StringIO except ImportError: from StringIO import StringIO

chipx86chipx86

I think you can use StringIO instead of this.

chipx86chipx86

ouput?

chipx86chipx86

I meant use StringIO here. We don't need OutputBuffer.

chipx86chipx86

Whatever is done here needs to be heavily commented. Can you instead just do: builtin = globals()?

chipx86chipx86
chipx86
  1. 
      
  2. rbtools/commands/rbconfig.py (Diff revision 1)
     
     
    Show all issues
    No need for the \, since the parens guarantee Python won't get confused.
    
    "by default," (with the comma) and not "by-default"
  3. rbtools/commands/rbconfig.py (Diff revision 1)
     
     
     
    Show all issues
    Seems like the set() math should be part of load_config_files instead.
    
    Once you've done that, you no longer need to convert this all into a dictionary. You'll just have one that you can iterate.
  4. rbtools/commands/tests.py (Diff revision 1)
     
     
    Show all issues
    Can you import rbconfig instead, and then call rbconfig.main()?
  5. rbtools/commands/tests.py (Diff revision 1)
     
     
    Show all issues
    ouput?
  6. rbtools/commands/tests.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    No need for re.split. You can just do str.split(delimiter).
    
    Also, we should avoid map and filter where possible. They're less readable and Python suggests not using them. Instead, this can be:
    
    [s.strip() for s in line.split(":")]
    for line in text.splitlines() if line
    
    You also don't really need to turn this into a dictionary. This whole thing can be greatly simplified:
    
    values = [[s.strip() for s in line.split(":")]
              for line in text.splitlines() if line]
    
    for key, value in values:
        self.assertEquals(value, ...)
    
    Actually... What's this even doing? It's just comparing itself with itself. So something is wrong here.
    
  7. rbtools/commands/tests.py (Diff revision 1)
     
     
    Show all issues
    Should be clear that it's testing in rb config.
  8. rbtools/commands/tests.py (Diff revision 1)
     
     
    Show all issues
    Should be clear that it's testing in rb config.
  9. rbtools/utils/testbase.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    I think you can use StringIO instead of this.
  10. rbtools/utils/testbase.py (Diff revision 1)
     
     
    Show all issues
    ouput?
  11. 
      
MB
chipx86
  1. 
      
  2. rbtools/commands/rbconfig.py (Diff revisions 1 - 2)
     
     
    Show all issues
    The "by default," wasn't addressed.
  3. rbtools/commands/tests.py (Diff revisions 1 - 2)
     
     
    Show all issues
    "Test the '-a/--all' option in rb-config"
  4. rbtools/utils/testbase.py (Diff revisions 1 - 2)
     
     
    Show all issues
    This needs to be:
    
    try:
        from cStringIO import StringIO
    except ImportError:
        from StringIO import StringIO
  5. rbtools/utils/testbase.py (Diff revisions 1 - 2)
     
     
    Show all issues
    I meant use StringIO here. We don't need OutputBuffer.
  6. rbtools/utils/filesystem.py (Diff revision 2)
     
     
    Show all issues
    Whatever is done here needs to be heavily commented.
    
    Can you instead just do: builtin = globals()?
    1. I think we discussed that on IRC. globals() values can intersect with those loaded from config and hence we will strike them out. exec gives as a 'pristine' copy of python environment. 
  7. 
      
MB
chipx86
  1. Looks good. Made a couple small grammatical fixes.
  2. 
      
MB
Review request changed

Status: Closed (submitted)

Change Summary:

Committed to api (4990e1d)
Loading...