E127 continuation line over-indented for visual indent

E126 continuation line over-indented for hanging indent

W391 blank line at end of file

oops forgot to include this in the other review but did you mean to include this in the RB? correct …

i think you can combine line 33 and line 34

wouldn't closest_path be better named as closest_paths? to make it clear it's multiple "closest paths" and not just one

do you need both of tool_name and closest_path? it looks like you're only using tool_name in the string you're passing …

i think 'path'])) should be on the previous line

i'm a little confused about the logic here - so i think you're waiting for a RB server URL to …

typo - double a

is there any particular reason why you used separate print statements here as opposed to a single print with a …

if closest_path is a list, wouldn't it make more sense to name it closest_paths instead? to make it clear it's …

i think this could be better described as A list of best-matching repositories from a valid Review Board server. - …

i think the comment could be clearer if it said something like A dictionary containing repository metadata.

is fields a dictionary? or a list? the list of unicode and rest of the comment seem to contradict one …

i think this extra line needs to be kept

i think this needs to be reverted

i think it would be better to use answers instead of values here - it wasn't clear to me whether …

i think this could be clearer - it sounds like the user has to pick a list as a response …

maybe this line could be replaced by raise ValueError? so that the catch block will be the only place you …

E501 line too long (80 > 79 characters)

F821 undefined name 'request_id'

E501 line too long (80 > 79 characters)

E501 line too long (82 > 79 characters)

E501 line too long (83 > 79 characters)

E501 line too long (83 > 79 characters)

not really familiar with .rst but i'm not understanding how .. _configuring: turned into Usage $ rbt setup-repo [options] in …

This spacing is a lil weird - could we get them to line up like before?

same comment as above about getting these line lengths to line up a bit better

I think 'rbt --help setup-repo' and 'https://www.reviewboard.org/docs/' could be inlined as opposed to using string format (the %s) since they're …

Could do for current_index in range(len(closest_paths)) instead as 0 is a default start parameter for range

The 1st line of the file says """Test for RBTools setup-repo command.""" but this line has Tests. Make it consistent …

I think you can remove the if

Could we seperate the contents of the unit test with a new line? That way it'd be like, where we …

I'm not quite sure on the style rules for these tests but could we get some more whitespace in here …

same comment about removing if - same goes for below tests

could use self.assertNone instead

could use self.assertTrue here

Can use self.assertFalse here

could use self.assertTrue here

could use self.assertTrue here

I think there's a typo in peforce - should be Perforce right? (not sure if the capitalization is needed but …

is this class providing something like a mock endpoint from which to retrieve repository URLs from? i'm not super clear …

Missing docstring

Rewording this line could help bring clarity more of that this function does. I think changing it to Accepted answers …

E501 line too long (104 > 79 characters)

E501 line too long (89 > 79 characters)

E501 line too long (99 > 79 characters)

E501 line too long (85 > 79 characters)

E501 line too long (89 > 79 characters)

E501 line too long (99 > 79 characters)

These labels are global throughout the entire docs, so we should be a bit more specific in the name here.

One blank line between the header and the text please. Here and below.

We can link to the relevant docs in the Review Board manual with :ref:rb:repositories

Let's add two blank lines above each of these sections.

Can we add a docstring for this please?

This can go on the previous line.

Please add a blank line above this one.

It's usually not great to put exclamation points in output text.

I'm not pleased with all of these print statements which start with a newline. Perhaps we can add the extra …

I know other comments in here aren't all correct, but new ones should be in the imperative mood rather than …

One space between the arg name and type, colon at the end.

Indent this 4 more spaces.

One space between the arg name and type, colon at the end.

Indent this four more spaces.

Instead of defining self._mock_input, you should be able to just use: self.spy_on(get_input, call_fake=lambda: '1')

The way this is wrapped is confusing (it wraps in the middle of inner parens, making it unclear that path= …

Same here re: wrapping.

unicode shouldn't be capitalized here. But if you eliminate this function entirely it doesn't matter :)

Please add a module docstring for this file.

This needs a docstring.

Wrapping is a little funky here.

Indent this 4 more spaces.

This should include a "Returns:" section.

This should include a "Returns:" section.

The python type is named "int"

Please add a blank line between these two.

I feel like we probably want to return int_answer here. The place which uses this immediately converts to int again.

Please add a blank line above this.

E128 continuation line under-indented for visual indent

I think we should probably make it more clear that this first sentence is only for RBCommons subscribers. For folks …

Looking around at the other commands, I think we tend to use print() in place of inserting \n characters. Example: …

Same as above, re: \n characters.

Same as above, re: \n characters.

If possible, let's try to be consistent about where we put spaces at line breaks. I believe the convention in …

Same as above, re: \n characters.

Same as above, re: \n characters.

Same as above, re: \n characters.

Same as above, re: \n characters.

Small nit, but let's have two blank lines here. We usually use 2 between sections, and the .. rbt-command-usage:: directive …

Let's define a reference for RBCommons, and then use RBCommons_ everywhere we reference it, so that it'll link. The reference …

"here" is almost always a bad choice for a label for an anchor, because screen readers can't really present that …

We can use :ref:rbtools-reviewboardrc to reference .reviewboardrc. It will have the right name, and it will link to the documentation …

We can nuke this last sentence if we have set up the links above properly.

Let's do two blank lines here.

Keys should all be in alphabetical order. Same with ones below.

This is missing a trailing comma.

This is missing a trailing comma.

We didn't have this before, but can you have a blank line between each variable definition? Helps to keep them …

This should be an absolute path.

Missing a trailing comma.

There's a tweak to this that I'd like to see. It was what I had in my head when we …

This would be a little more clear as: """Initialize the base functionality for the command.

This must use the absolute path to the type, so rbtools.api.transport.Transport. It must also have , optional after the type, …

I'd suggest just: The transport class used for all API communication. By default, this uses the transport defined in :py:attr:`default_transport_cls`. …

After the above changes, this would be self.transport_cls.

The % should be on the last line.

We're eventually going to want to localize our code, and we can get a head start by using a different …

Blank line between these.

Blank line here (the return is the conclusion, and not part of the outputting block of code, so it gets …

There's a lot of strings like these that are going to be pretty long, and they might not look good …

The % CONFIG_FILE should go on its own line.

Comments should use proper sentence structure (capitalization, punctuation, "the", etc.).

"URL" "neither api_root nor ..."

"repository" instead of "repo". Helps in comments for those still learning terminology/code.

You'll need to test this part a bit more, because it will break. When passing multiple values to %, you …

Can you use backticks around the rbt --help setup-repo? Maybe even just do rbt help setup-repo, to keep it a …

This should be formatted like: question = ( '.......' % (outfile_path, output) )

When you have code that needs to iterate through something and return both an index and a value, it's best …

Can we use %d instead of %i? There isn't much of a difference, but we've standardized on %d, and consistency …

Just like above, let's prepare for localization and use %(blah)s arguments and a dictionary.

For os-level commands, we prefer to import os and then reference as os.path, etc. This avoids common problems with redefining …

These aren't needed, since you're not doing anything other than calling the parent.

It's best to group things into paragraphs, to avoid code just all blurring together. Spying on functions is a paragraph. …

Can you call this using keyword arguments, to help with readability and future potential changes? Same below.

You can use assertIsInstance.

fp is commonly used for with open(...), and thus will be a little more familiar and readable.

It's best to format these things like: self.assertEqual( config_lines, [ '...', '...', '...', ]) Helps break them apart, keep them …

Missing a trailing comma.

Let's pull out setup.options into a variable, so we can condense all this, reduce the noise. Same with other tests.

+= is actually a little faster than .extend (operators generate different opcodes and are invoked in different ways than function …

Blank line between class docstrings and members.

Missing , optional.

When referencing classes, we use: :py:class:`absolute.path.to.Class` For modules, use :py:mod:.

Missing , optional.

Same notes as above.

We shouldn't use backticks in the summary. It can turn out wonky. We also really want to describe here what …

These should use :py:class: to reference these classes.

When referencing an attribute on the class, use: :py:attr:`list_payload`

Missing docs for *args and **kwargs.

Should use :py:class:.

This must be an absolute class path.

API requests always use absolute URLs.

Blank line between these.

Same comments as above. Also, there's some leading whitespace here.

Use :py:meth: to reference methods. Same comments otherwise apply to these docs.

This should be an absolute path.

This can be one statement.

No blank line here.

E126 continuation line over-indented for hanging indent