Lindsey / Brendan:

Thanks for posting this up!  I've broken my review down into two sections - Style, and Architecture:

***Style***

So, style-wise, here are the big ones:

1)  Extraneous whitespace has to go.
2)  Lines must be less than 80 characters
3)  Formatting for docstrings needs some fixing:  see http://github.com/reviewboard/reviewboard/blob/master/reviewboard/webapi/resources.py for examples
4)  Code blocks outside of classes are spaced with two blank lines.

A lot of this style stuff can be caught using the pep8 script, which you can install with easy_install.  Very useful.

So I didn't highlight everything style-wise, otherwise there'd be a ton of comments for you to go through.  Suffice it to say, pep8 will help you nail a huge chunk of them.

***Architecture***

It's a good start.  A suggestion (and Christian/David might override me on this, but I thought I'd toss in):

We might want to organize these parts into different modules, like:

rbtools.api.client (for the client...maybe the REST interface, too)
rbtools.api.resources (all of the RESTful resources)
rbtools.commands (this could maybe have a bunch of modules in it - one for each command.  This is how the Django framework does it.)
rbtools.utils (maybe have a separate module for the SCM utils...)

I don't know.  Hugely open to discussion on that.  Christian/David/others might have a better idea about how to organize something like this.

What I'm pretty sure we'll want to do, though, is do imports somewhat like the following:

from rbtools.api.resources import ReviewRequest, ReviewRequestDraft,...

This means that we can instantiate ReviewRequests without the Resources prefix.

I'm curious:  why does get_repository in RBUtilities have so much flexibility in terms of what SCMClients it looks for?  Just wanted to know.  :)

Anyhow, that's what I've got from a first glance.  Might follow up on this with more once I get my midterm done.

Should explicitly inherit from object, like

class RBUtilities(object)

No space needed here, or on line 27.

It might be better to have a list of SCMCLIENTS (or a dict, if you want to keep the keys) instead of writing this out statically.

The original postreview code did this, and it seemed to work pretty well.

import RBUtilities should be put after import os.

We do imports alphabetically.  If there's an unordered list (like imports, or css attributes), we *tend* to put things in alphabetical order.

alphabetize

If you're outside of a class, there are two lines between code blocks.

So we'd insert 2 lines between the imports and the try/catch.

extra line here.

add another extra line here.

and add another extra line here.

No need for this line.

alphabetical order, and two lines before try/catch

add another line here

And add another extra line here.

alphabetize

Hi guys,

This is going to be a long review. You knew that was coming, but don't worry :)

There are a lot of stylistic things that still need a lot of love. I listed some examples of this but I didn't go through every line to comment on each occurrence. Please go through your code and make sure that everything follows the styles as expected. This will make it *much* easier to review logic and implementation rather than style.

As for the structure of the code, I really want to see different types of classes segmented into namespaces. There should be the following:

* rbtools.api -- All API-related things should go somewhere in here. Resources, for instance, should go in rbtools.api.resources (or rbtools/api/resources.py).
* rbtools.clients -- All support for clients (git, svn, etc.) should go in here. This is what you currently have as the repository classes. I want to see one file per type of repository. So, rbtools.clients.git (or rbtools/clients/git.py) would have GitRepository and GitClient.
* rbtools.commands -- All scripts would move into here, and you'd use the Python setuptools scripting stuff to wrap these. See RBTool's setup.py for how this is done with post-review.

On that note, filenames should always be lowercase in Python. The mixed-case style of Java, C++, etc. isn't used here.

I want to see some changes to the way Resources are done. Basically, I want to be able to treat a Resource as a sort of structure I can get/create, populate, and save, and have it work sort of like the Django database API. Something like:

review_request_list = ... # Pretend we have this at this point
review_request = review_request_list.create()
review_request.submit_as = some_user
review_request.repository = repository_id
review_request.save()

In a way, it's like the Django database API. I should also be able to fetch a resource, change some stuff on it, and save(). If the review request is newly created, do an HTTP POST. If it's one fetched from a server, do a PUT. Should also provide a delete() method. Etc.

So, basically, there would be two main types of resources: Lists, and objects.

List resources would have create() and get() methods.

create() would create a new instance of the type of child resource, which could be populated and saved.

I'd like a get() that would get the correct resource from the list. The only way this can be reliably done (since an object doesn't necessarily live in that resource, as it may be linked to another place) is to query the root resource (which should only be done once in the lifecycle of the app), find the uri_template for the resource name (say, 'review_requests'), and plug values into the URI template (review_request_id, etc.). Those values should come from values passed to get() and from the list resource's values (and its parents, recursively up the chain).

So, I could do 'review_request_list.get(42)' and it would plug 42 into review_request_id. It would know this from, say, an 'uri_id_field' key in the resource subclass. It would get any info from the list, its parent, etc., and then once it has all this, parse the URI template and get the URL. Then fetch that and return the resource type.

Resource lists would also need to be able to return their results in a nice, Pythonic way involving iterators. I want to be able to do:

    for resource in resource_list:
        ...

and:

    resources = resource_list[4:10]


Resource objects would have save() and delete() methods. Fairly self-explanatory I think.

For all this to work, you need two classes (they should have a base parent class that has most of the logic for this, though), and then a subclass for each thing that we care about.

ResourceList subclasses would probably need:

* child_resource_type (reference to a class, such as ReviewRequest).
* create()
* to be iterable (there should be lots of examples of this on the web).


Resource subclasses would need:

* uri_id_field
* resource_name
* save()
* delete()

That's probably a lot to process. Feel free to discuss it further over e-mail on reviewboard-dev@.

Imports are always in three groups, each separated by a blank line:

1) System imports (such as sys, mimetools, socket, etc.)
2) Third-party imports (would be things such as djblets, though that's not needed here).
3) Local imports (in this case, Repository).

For local imports, use the full import path. In this case, 'from rbtools import Repository'.

Classes should always inherit from object, like so:

class RBUtilities(object):

No blank lines on the insides of the """

Make sure everything fits under 80 columns.

No blank line.

Functions should always be more like:

def __init__(self, log_file='rbproblems.log'):

Note the lack of spaces within the parens, and the lack of spaces around the '='. This is true only for keyword arguments, not general assignment in code.

Docs in Python shouldn't contain the function header (the __init__(...)) or list of parameters.

The very first line should always be the summary, like:

"""Briefly describes the function.

More detail that can span paragraphs here.
"""

Though, for init functions, don't bother with docs.

No blank line immediately after the docs.

Only one blank line between functions within a class.

Don't define a long list of things in the header. This should go outside of the function somewhere.

Always wrap to < 80 chars.

It would be better to have a lookup map somewhere outside the function, like:

repository_types = {
    'svn': Repository.SVNRepository,
    'cvs': Repository.CVSRepository,
}

Then you can do:

if type in repository_types:
    possible_repos.append(repository_types[tpe](url, self))
else:
    self.raise_warning(...)

No blank line here.

Parameters must always be aligned.

No blank line right after the function declaration.

You should never need this. It's deprecated. All string operations can be performed on strings themselves.

We can dictate the return type, so there should never be a need to handle anything but json.

Why the 1? This should be documented.

Rather than doing this, why not just set self.url so we can access resource.url?

When comparing against None, use "is":

if field is None:

Actually, do this instead:

.. note:: This method MUST blah blah
          blah blah

It'll appear nicer in generated docs.

No need for \ when inside parens.

Why the couple random spaces? What happens if e.msg doesn't end with a period?

You can make a lot of this much easier.

Require that subclasses define the name of the resource field within, like so:


class ReviewRequest(Resource):
    resource_name = 'review_request'


Then have Resource be smart enough to do the get_field.

Rather than things like draft_url, just provide an easy way to get a named URL. I think having get_link jut take the name and return the href instead of requiring the ['href'] would do that nicely.

Align these, and no need for \.

Also. there's an easier way here. Define this as:

def __init__(self, method, *args, **kwargs):

Then, for the parent constructor call, do:

super(RequestWithMethod, self).__init__(self, *args, **kwargs)

That way, you never have to keep the params in sync with urllib2.Request.

Align these, and no need for \

Should use:

return super(RequestWithMethod, self).get_method()

No need for \

The final ] should appear on its own line.

No need for \, and align params.

I don't think there's much point in this. We should always just use json.

Align params.

No need for \, and align params.

Login in the new API shouldn't use this. It should use HTTP Basic Auth.

Paths should never be hard-coded with the REST API. They should be "discovered" or "browsed" from the root resource.

No need for \, and align params.

Don't use \ in the strings. Instead, terminate a string and resume it on the next line.

These methods (the ones operating on review requests and such) don't really belong in here. The act of creating or updating an object should happen solely on the Resource for it.

Multi-line if statements should have the conditional wrapped in parens instead of using \.

In this case, you can do:

if not isinstance(self.current, (Resource.ReviewRequest, Resource.DraftReviewRequest):

More stylistic stuff, and some code simplification/reorganization.

Remember, the filenames should be all lowercase, not mixed case.

There are still things from previous reviews that I think are unfixed. When I'm modifying my change based on review feedback, I'll usually respond to each item with "Fixed" or something, so that the reviewers and I all know what was done and what wasn't.

Going to want David's take on this, but I generally would say that you should have a base ResourceError exception, and then simple subclasses for each error type, instead of error codes. So:

class ResourceError(Exception):
    ...

class UnloadedResourceError(ResourceError):
    pass

etc.

Also, any exceptions should go in errors.py.

This should be:

Exception.__init__(self, *args, **kwargs)

Instructions probably don't belong in here. I'd do:

return self.resource_string or "Unloaded resource"

First line of any doc string should be a one-sentence summary, ideally fitting on one line. It should be on the same line as the starting """. Then, a blank line, then the remaining docs.

Should be consistent with where the space is on multi-line strings. I always put it before the end quote mark.

Also, any time you're breaking apart a string, all parts should be on the same indentation level. That includes "The resource"

Blank line.

I'm not convinced this is safe. You're using field both as the result and the thing you're fetching from. So what happens after the first iteration?

if field is None

"if field is None"

Same here about the indentation of the string.

Same here with the error string.

Also, any others.

Shouldn't print. This may be used from non-console apps. Callers should be able to get something meaningful so they can handle it their own way.

Should use:

super(ResourceBase, self).__init__(server_interface, ...)

You'll notice that's different from what I said to do about the Exception above. The reason is that new-style objects (those inheriting from 'object' somewhere in the chain) can use super(). Exception, however, isn't a new-style object on older versions of Python that we support.

Space after the '#'

Same with the other comments.

Kind of an odd way to do it. You can just do: if elem != 'stat':

Same here with the prints.

The ".. note:" is only for doc strings. Just use "NOTE: " here

Also, this seems very inconsistent with the other functions. We should be consistent, one way or another. The fewer types of exceptions the caller has to care about, though, the better.

You should make url_field_ids set to [] by default, and then just append the get_field('id') to the list always.

Loads

Space after the #

We should normalize the value passed to get_field() and then call it only once.

Space after #.

Same with all other comments.

Shouldn't print.

Where does is_resource_list come from?

Shouldn't print.

Should have some docs. Otherwise, just remove the """

use super()

Blank line between these.

Two blank lines between classes.

Same comment about the docs.

Same about super().

Should go through and make sure it's used right everywhere.

Comments like these aren't very useful. When code is self-explanatory, comments aren't needed.

Should be 'reopen'

There are two ways to close a review request -- to mark it submitted, and to discard it. You should define constants for PENDING, SUBMITTED, CLOSED, and use those. They can be defined to "pending", "submitted", "closed", but that way strings aren't assumed to be used.

ResourceList is a pretty fundamental base class. I'd put it before any specific resource classes.

Same about the comment.

Load

Blank line.

These comments aren't that useful either.

Parameters should always be aligned.

Maybe do:

    self.child_resource_url = \
        self.get_field(...)

Shouldn't print.

Should be one statement.

Can do:

reverse_url_field_ids = reverse(self.url_field_ids)

Shouldn't print.

Should do '%s{some_id_field}%s' % (temp[0], temp[2])

I don't really know what all this is for, though.

Shouldn't print.

Printing.

At this point I'm going to ignore any further prints.

I'd go through and remove all comments that don't explain otherwise confusing, complicated code.

Build the values to pass first, then call build_resource_url and Resource()

Should use a comment here, since it's not a function or class's documentation.

What's the reason for this? Shouldn't __next__ suffice?

== len(self)

Can collapse this:

elif self.is_root:
    ...
else:
    ...

Should also be a comment.

You can do without the 'contains' variable.

for n in self:
    if n == key:
        return True

return False

Since the root resource is special, it should probably just provide special versions of these iterator functions, instead of having every resource checking if it's the root.

No need for this blank line.

Same here about using a comment.

Are these meant to be used outside of this file? If not, then the names should have a _ prefix.

out isn't used until later, so no need to declare it yet.

I'm really not sure what any of this is supposed to do. This would be where a nice explanatory comment would be helpful. My gut is that there's an easier way to write it, but a comment would help me figure that out :)

Should use format strings for this. You can get it down to one statement.

field_id_index -= 1

Sorry to make you kill off this function, but Python has what you probably need here.

Python's 're' module (regular expressions) lets you do pattern-based splitting easily.

You can do:

import re
left, center, right = re.split('{([^}]+)}', 'foo{abc}def')

The pattern there matches something starting with { and anything up to (but not including) the }.

Result in that case would be: ['foo', 'abc', 'def']

Ah, this is what I was wondering before.

Isn't data a dictionary? If so, you can just do:

return 'total_results' in data

When importing, you want to first have a group of built-in Python modules, then a blank line, then 3rd party modules + blank line (if any -- none in this case), then imports from this module (in this case, the rbtools ones).

This can be simplified to:

return self._method or super(RequestWithMethod, self).get_method()

This is only used for the old-style API.

This should probably be split off into some little utility thing in the rbtools.commands API. A graphical third-party app would never want the library prompting for input.

Two blank lines.

We shouldn't use a log file here. We should use python's logging module.

This doesn't have to be done in the first change, but it really shouldn't hard-code the names. What it should be doing is looking for a program with the name of sys.argv[1] with 'rb-' prepended. That way, third parties can make their own and it'll Just Work.

But again, that's a future task, which isn't a priority over getting this change in.

Two blank lines.

Two blank lines.

Two blank lines.

Two blank lines.