This won't work for Windows.

Need to use os.path.join() to build paths.

Blank line between these.

Can't use the "as" form.

Can you switch this to using Python format strings while you're at it? It'll be important for localization later.

Here too.

Should only be indented 4 spaces, relative to "values."

The trailing } should be aligned with "values".

"Filter out the values that are None."

No space after "[" and before "]".

Blank line before this.

You can just do:

    if valid:

You can just do:

    for name, value in valid:

Blank line between these.

These won't work with Python 2.4. It should stay how it was. Same with the ones below.

Isn't this a 3.0 thing?

Alexander:

Hey - this patch appears to be broken.  Can you try updating it?

Thanks,

-Mike

Where are the call sites of these methods you're removing?  Are there none?

By the way, I can't see the branch you refer to on github.  Is it on a fork?  Which one?  What is the branch for?

If you pass in the config file paths rather than putting this code in the constructor, the tests don't need to rely on the current working directory (nor on conventions about cwd, ~, etc. that might change over time and aren't relevant to most of the tests).

If you want to test that the current working directory is used as a default, you can test that separately in a single test.  Since it's nice to have the minimal amount of code possible depend on such global state, it might be better to move the getcwd() call out of this class (especially since that could change).

Nice talk about this:

http://www.youtube.com/watch?v=-FRm3VPhseI

Every time you make a Settings object you immediately call .load on it -- except here.  Is that a mistake?

Why not just let Settings load when it needs to (e.g. in the constructor)?

It's nice that this is an explicit method rather than ad-hoc code in the tests.

It would be even better if it cleaned up at the end of the test case (by removing the temporary directory -- and chdir-ing back to the original cwd, if the chdir is really necessary).  The same goes for the other methods here that touch global state.  One nice way to do that is to implement these using a method similar to unittest2's .addCleanup().

Here's a really simple implementation of that idea:

http://lackingrhoticity.blogspot.com/2008/11/tempdirtestcase-python-unittest-helper.html

Dead code?

Since you're cleaning this up, maybe add a usage string to say what the program does?

Isn't it "Review Board", not "ReviewBoard"?

(I don't know for sure)

Needs a help string.

Why not make the option dest the same as the config file here?  It seems confusing to present the user with two names for the same setting.

If you do that, you can use optparse.make_option (and the .dest attribute of optparse.Option), which would allow eliminating the repetition of the dest values.

Use "is not None", not "!= None"

If it's going to print anything at all on success, maybe move this to after the setattr and have it print something that explicitly says what it has done, e.g. "Set %s : %s"?

This isn't very helpful to the user:

$ rb config

Usage: rb config [options]


Maybe print_help() instead?

This is a no-op, right?  Delete the try/except?

Alexander:

This is looking really solid to me!  Just a few nits, and some documentation requests.  Thanks,

-Mike

typo:  "<name> is a valid option" - also, instead of two block quote passages, maybe this could be a single?  See:  https://github.com/reviewboard/reviewboard/blob/b5209aabf67994f8613844fa20dbdd55eab07a87/reviewboard/webapi/resources.py#L810 for example.

Also, please properly capitalize in your documentation.

Not sure I understand this documentation.  How about just "First load global options, and then local options.

Also, proper capitalization. 

Needs proper capitalization.

I think some quick documentation on what each of these methods tests would be fantastic. :)

See above, re single block quote.  Maybe for this, you can put:

"""Tests that we can set arbitrary attributes on an instance of Settings.
"""

Some documentation would be great here, as well as in the class methods.

Space before the if block

Needs documentation - just a quick sentence on what this class does.  If you could toss in some documentation for the methods as well, that'd be awesome. :)

So there's a number of specific style comments, but I have a few main points:

1) This seems to use a lot of old code (RBUtilities, for example) instead of the newer modules that you put into place with the SCMClients change. It doesn't look like it's merged with master?

2) I'm concerned this change is doing too much. It's fixing style issues, modifying stuff in SCMClients, adding a whole new settings framework, adding unit tests, adding unrelated TODOs (for git). It's the perfect example of a change that should be split up.

I'd advise splitting up the settings stuff into its own change, after fixing the things below, and having another change solely for the style changes and comment additions.

I'd also advise having a change just for the new unit test utils.

I know that seems like extra work when you already have it all written, but we're going to have to get things in little by little, and when you have a really large change and we notice a regression or problem down the line, it makes it far more difficult to track it. Bite-sized changes that implement one thing are way easier all around.

As a more general comment, if a change can be put onto master (style changes, maybe unit test utils), it'd be good to target it there instead of api. That results in less we end up merging in from api, making that easier to maintain as well.

Important note: This can't be required, or we'll break a *lot* of companies. I don't know how ConfigParser works, but if it absolutely has to have an INI section header, we can't use it here. Actually, come to think of it, we can't use ConfigParser. We need the .reviewboardrc files to work as they always have been. That is, they need to be essentially Python scripts. Many companies depend on them to be executable in order to fix up proxy issues and stuff. So that can't change.

Blank line after this.

No need for \

Blank line after this.

Should use super.

Should use sentence casing, and have a period.

Blank line between these.

Blank line between these.

Blank line between these.

No need for parens.

Blank line after this.

Blank line between these.

Only one space after import.

Needs to be one line, and should be short.

No blank line here.

Didn't think we still had this. Or we shouldn't anyway. Like the SCMClients, it should import straight from the module it's in.

Same with all other instances.

assertRegexpMatches isn't in Python 2.4.

Can't depend on with. It's only in newer Pythons than we support.

No blank line here.

Should be two blank lines here.

Indentation is broken.

Probably didn't mean to leave this in?

This seems... Very weird. Can't it just be:

for name in opts:
    uuid = self.gen_uuid()
    args.append(...)
    ...

Can you explain some of the motivation for this change?  Why the change from utils to util? And files to fs?

It seems like there are things in here that probably weren't supposed to be part of this review request.

Should probably use parens.

Should use parens.

Should use parens.

If this is going to be used by code outside of this class, it should be a public function (no _ prefix), and shouldn't be "internal."

However, this seems to be something that should be in a different review request, since it's not really cleanup, is it?

Should use parens.

This would actually be better as a pre-compiled regex constant as part of the class, like:

DATE_RE = re.compile(r'....')

...

m = self.DATE_RE.search(dl[1])

Are these intended to be in this change?

Alexander:

This looks pretty good!  I agree with Christian, however - this change should probably be broken up into two or more diffs.  I've noticed that some changes are purely cosmetic / stylistic (cleaning whitespace, reducing lines to <= 80 chars), while other changes have deeper potential impacts (renaming, new code, etc).  Maybe you can split along those lines.

All the best,

-Mike

Should be "accessed".

Space after #, and capitalize first letter of first word.  Same for comments below.

I'm not 100% sure we need this success variable.

The only time it's ever used, it's guaranteed to be True.

I think the var can be dropped, and just return true on line 410.

Wait - hold up, why can't we commit?

Linebreaks before and after this if block.

Linebreak after this else block

Linebreak before this if-block

Except for the one issue below, this looks pretty good to me.

Linebreak after this if block

Just about there! One small style things, and a couple critical problems.

print isn't a function, and unlike if statements, we tend to just use \

print "No supported ".... \
      "supplied url."

(Always break on full words.)

The compile should be a constant on the class, like:

DATE_RE = re.compile(...)

Also, "compile," not "complie".

This isn't part of your original change, but while you're here, "json" is only on Python 2.6. So instead, do:

try:
    from json import dumps
except ImportError:
    from simplejson import dumps

Ship It!