Make rbt setup-repo more clear

Review Request #10894 — Created Feb. 7, 2020 and submitted

kpatenio
RBTools
release-2.0.x
4624915...
rbtools

This change makes the setup-repo command easier to follow and understand
by adding more information concerning the purpose of the command, how to
configure repositories, and the overall status of the config file
creation process.

The repository selection process has been simplified;
multiple repositories can be seen at once, and the repository of choice
can simply be selected without having to go through other repositories.

Unit tests have also been added for the setup-repo command to ensure
functionality in future releases.

I have detailed notes on my current changes here.

Test cases run on python 2.7.17 and 3.7.6:
- Test case validating repository prompt if repositories exist
- Test case validating repository prompt if no repositories exist
- Test case validating config file generation
- Test case validating the contents of config file
- Test case validating server options
- Test case validating perforce options
- Test case validating tfs options

Loading file attachments...

Description From Last Updated

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

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

hxqlinhxqlin

i think you can combine line 33 and line 34

hxqlinhxqlin

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

hxqlinhxqlin

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 ...

hxqlinhxqlin

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

hxqlinhxqlin

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

hxqlinhxqlin

typo - double a

hxqlinhxqlin

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

hxqlinhxqlin

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

hxqlinhxqlin

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

hxqlinhxqlin

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

hxqlinhxqlin

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

hxqlinhxqlin

i think this extra line needs to be kept

hxqlinhxqlin

i think this needs to be reverted

hxqlinhxqlin

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

hxqlinhxqlin

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

hxqlinhxqlin

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

hxqlinhxqlin

E501 line too long (80 > 79 characters)

reviewbotreviewbot

F821 undefined name 'request_id'

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

E501 line too long (104 > 79 characters)

reviewbotreviewbot

E501 line too long (89 > 79 characters)

reviewbotreviewbot

E501 line too long (99 > 79 characters)

reviewbotreviewbot

E501 line too long (85 > 79 characters)

reviewbotreviewbot

E501 line too long (89 > 79 characters)

reviewbotreviewbot

E501 line too long (99 > 79 characters)

reviewbotreviewbot

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

hxqlinhxqlin

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

hxqlinhxqlin

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

hxqlinhxqlin

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 ...

hxqlinhxqlin

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

bui1bui1

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

bui1bui1

I think you can remove the if

hxqlinhxqlin

Each of these docstrings needs a period at the end

bui1bui1

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

bui1bui1

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

hxqlinhxqlin

same comment about removing if - same goes for below tests

hxqlinhxqlin

E501 line too long (104 > 79 characters)

reviewbotreviewbot

could use self.assertNone instead

bui1bui1

could use self.assertTrue here

bui1bui1

Can use self.assertFalse here

bui1bui1

could use self.assertTrue here

bui1bui1

could use self.assertTrue here

bui1bui1

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

hxqlinhxqlin

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

hxqlinhxqlin

E501 line too long (89 > 79 characters)

reviewbotreviewbot

E501 line too long (99 > 79 characters)

reviewbotreviewbot

Missing docstring

hxqlinhxqlin

E501 line too long (85 > 79 characters)

reviewbotreviewbot

E501 line too long (89 > 79 characters)

reviewbotreviewbot

E501 line too long (99 > 79 characters)

reviewbotreviewbot

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

bui1bui1

E501 line too long (104 > 79 characters)

reviewbotreviewbot

E501 line too long (89 > 79 characters)

reviewbotreviewbot

E501 line too long (99 > 79 characters)

reviewbotreviewbot

E501 line too long (85 > 79 characters)

reviewbotreviewbot

E501 line too long (89 > 79 characters)

reviewbotreviewbot

E501 line too long (99 > 79 characters)

reviewbotreviewbot

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

Can we add a docstring for this please?

daviddavid

This can go on the previous line.

daviddavid

Please add a blank line above this one.

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

Indent this 4 more spaces.

daviddavid

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

daviddavid

Indent this four more spaces.

daviddavid

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

daviddavid

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

daviddavid

Same here re: wrapping.

daviddavid

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

daviddavid

Please add a module docstring for this file.

daviddavid

This needs a docstring.

daviddavid

Wrapping is a little funky here.

daviddavid

Indent this 4 more spaces.

daviddavid

This should include a "Returns:" section.

daviddavid

This should include a "Returns:" section.

daviddavid

The python type is named "int"

daviddavid

Please add a blank line between these two.

daviddavid

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

daviddavid

Please add a blank line above this.

daviddavid

E128 continuation line under-indented for visual indent

reviewbotreviewbot

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

mike_conleymike_conley

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

mike_conleymike_conley

Same as above, re: \n characters.

mike_conleymike_conley

Same as above, re: \n characters.

mike_conleymike_conley

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

mike_conleymike_conley

Same as above, re: \n characters.

mike_conleymike_conley

Same as above, re: \n characters.

mike_conleymike_conley

Same as above, re: \n characters.

mike_conleymike_conley

Same as above, re: \n characters.

mike_conleymike_conley

Same as above, re: \n characters.

mike_conleymike_conley

So if they say no, do we just ask again? Remind me again - was it a conscious choice on ...

mike_conleymike_conley

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Let's do two blank lines here.

chipx86chipx86

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

chipx86chipx86

This is missing a trailing comma.

chipx86chipx86

This is missing a trailing comma.

chipx86chipx86

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

chipx86chipx86

This should be an absolute path.

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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`. ...

chipx86chipx86

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

chipx86chipx86

The % should be on the last line.

chipx86chipx86

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

chipx86chipx86

Looking through some of the other RBTools files, I think the convention for formatting here is: printf('Some formatted string: %s, ...

mike_conleymike_conley

Blank line between these.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

The % CONFIG_FILE should go on its own line.

chipx86chipx86

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

chipx86chipx86

"URL" "neither api_root nor ..."

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

You can use assertIsInstance.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Blank line between class docstrings and members.

chipx86chipx86

Missing docs for *args and **kwargs.

chipx86chipx86

Missing , optional.

chipx86chipx86

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

chipx86chipx86

Missing , optional.

chipx86chipx86

Same notes as above.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Missing docs for *args and **kwargs.

chipx86chipx86

Should use :py:class:.

chipx86chipx86

This must be an absolute class path.

chipx86chipx86

API requests always use absolute URLs.

chipx86chipx86

Blank line between these.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

This should be an absolute path.

chipx86chipx86

This can be one statement.

chipx86chipx86

No blank line here.

chipx86chipx86

E126 continuation line over-indented for hanging indent

reviewbotreviewbot
kpatenio
kpatenio
kpatenio
Review request changed

Branch:

-release-2.0.x
+setup-repo-fix

Commit:

-79c30c7acfb24eeb33f51c43ecc10096cd8ac3d1
+7e38198b2eca4fac292015e6633ced68c83c35d0

Diff:

Revision 2 (+140 -7)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

kpatenio
kpatenio
Review request changed

Change Summary:

Updated repo prompt and descriptions + added tests

Summary:

-[WIP] Make rbt setup-repo more clear
+Make rbt setup-repo more clear

Description:

~  

Not ready for review!

  ~

This change makes the setup-repo command easier to follow and understand

  + by adding more information concerning the purpose of the command, how to
  + configure repositories, and the overall status of the config file
  + creation process.

   
~  

This is a fix to make the purpose of rbt setup-repo command easier to

~   understand. The instructions, errors, and other relevant outputs will
~   simply be updated.

  ~

The repository selection process has been simplified;

  ~ multiple repositories can be seen at once, and the repository of choice
  ~ can simply be selected without having to go through other repositories.

   
~  

As an addition, adding unit tests for setup-repo is currently in my

~   roadmap for this project.

  ~

Unit tests have also been added for the setup-repo command to ensure

  ~ functionality in future releases.

   
~  

I have notes on my current changes here.

  ~

I have detailed notes on my current changes here.

Testing Done:

  +

Test cases run on python 2.7.17 and 3.7.6:

  + - Test case validating config file generation
  + - Test case validating the contents of config file
  + - Test case validating server options
  + - Test case validating peforce options
  + - Test case validating tfs options

Commit:

-9ac4312f3ec3aa29748f38418461ed602fbc344d
+9eee524a588a303f8527c4edd988f728c132ff8d

Diff:

Revision 4 (+264 -45)

Show changes

Removed Files:

Added Files:

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

kpatenio
kpatenio
  1. 
      
  2. For reviewers

    • what are your thoughts on my current changes?
    • are the descriptions detailed enough?
    • should I update the docs?
    • can I do more with my unit tests?

    Also, I highly recommend you look at my Notion notes here. They go in depth on my changes.

  3. 
      
kpatenio
kpatenio
hxqlin
  1. 
      
  2. looking good! i have a comment about the screenshots - i'm not sure if there's a direct link between the before/afters but if there is, it would be helpful if you could name them consistently so it's easier to compare the before and after. for example, if you have a screenshot called Before: no repo found or selected it would be a lot easier to compare the after screenshot if it was named After: no repo found or selected

  3. rbtools/commands/setup_repo.py (Diff revision 6)
     
     

    i think you can combine line 33 and line 34

  4. rbtools/commands/setup_repo.py (Diff revision 6)
     
     

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

  5. rbtools/commands/setup_repo.py (Diff revision 6)
     
     

    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 to confirm_select

    1. I realized that I wasn't really using closest_path. I updated the confirm_select function and replaced the second the parameter to represent the length of closest_path.

  6. rbtools/commands/setup_repo.py (Diff revision 6)
     
     
     

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

  7. rbtools/commands/setup_repo.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     

    i'm a little confused about the logic here - so i think you're waiting for a RB server URL to be entered and you validate it inside the loop. once you validate it, it looks like you're checking whether api_root and api_client exist again on line 142 and retrieving it if they don't. does it need to be checked twice?

    1. I've added some comments in the code:

              # If we run the `--server` option or if we run
              # `rbt setup-repo` with a server url already defined in our config
              # file, `api_root` nor `api_client` will be defined. We must revalidate
              # our server again.
      
  8. rbtools/commands/setup_repo.py (Diff revision 6)
     
     

    typo - double a

  9. rbtools/commands/setup_repo.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    is there any particular reason why you used separate print statements here as opposed to a single print with a long string like on line 115? if we do need separate print statements here, is it possible to move line 159 and line 168 to the previous lines so that the formatting is consistent?

  10. rbtools/commands/setup_repo.py (Diff revision 6)
     
     

    if closest_path is a list, wouldn't it make more sense to name it closest_paths instead? to make it clear it's plural and not just a single path

  11. rbtools/commands/setup_repo.py (Diff revision 6)
     
     
     
     

    i think this could be better described as A list of best-matching repositories from a valid Review Board server. - the current comment sounds like closest_path is an object that contains a list of unicode

  12. rbtools/commands/setup_repo.py (Diff revision 6)
     
     

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

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

  14. rbtools/utils/console.py (Diff revision 6)
     
     

    i think this extra line needs to be kept

  15. rbtools/utils/console.py (Diff revision 6)
     
     

    i think this needs to be reverted

  16. rbtools/utils/console.py (Diff revision 6)
     
     

    i think it would be better to use answers instead of values here - it wasn't clear to me whether values was referring to parameters of the function or the answers

  17. rbtools/utils/console.py (Diff revision 6)
     
     

    i think this could be clearer - it sounds like the user has to pick a list as a response and not just one of the options. maybe something like The list of available options that the user can choose a response from.

  18. rbtools/utils/console.py (Diff revision 6)
     
     

    maybe this line could be replaced by raise ValueError? so that the catch block will be the only place you need to print this string

  19. 
      
hxqlin
  1. 
      
  2. .reviewboardrc (Diff revision 6)
     
     
     
     

    oops forgot to include this in the other review but did you mean to include this in the RB? correct me if i'm wrong but this looks like something that is just needed for local setup

    1. Yep, Hannah is right - I don't think this change is needed (I believe it'll change the default of how rbt land will work on the rbtools repository).

    2. If I want my changes to be in release-2.0.x, wouldn't I need to change my config file? (I also just realized that I might have accidentally based my branch on master. Would this cause problems in the future?

  3. 
      
kpatenio
kpatenio
Review request changed

Change Summary:

Removed exit from prompt + reverted config file to try fix issues with patch changes appearing in revision?

Branch:

-master
+setup-repo-fix

Commit:

-795e8cb534b61da9c148a40cc80e9c5e6cbc67c2
+1836c100e5c368a1a394355ed0524f8a7099e2a1

Diff:

Revision 8 (+438 -466)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

kpatenio
kpatenio
kpatenio
kpatenio
kpatenio
kpatenio
kpatenio
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

kpatenio
Review request changed

Change Summary:

Minor syntax change + updated commit message

Testing Done:

   

Test cases run on python 2.7.17 and 3.7.6:

  + - Test case validating repository prompt if repositories exist
  + - Test case validating repository prompt if no repositories exist
    - Test case validating config file generation
    - Test case validating the contents of config file
    - Test case validating server options
    - Test case validating peforce options
    - Test case validating tfs options

Commit:

-f97923ea194e5f041c7632ed00947aa3f1854f5a
+3a3894253da90804e714d1ef78531b4d3ac94fbe

Diff:

Revision 14 (+445 -51)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

kpatenio
Review request changed

Change Summary:

Some whitespace changes to a print statement

Commit:

-3a3894253da90804e714d1ef78531b4d3ac94fbe
+64a71292f04fb0a5748f4d48cb0fcd2f3cdf18f9

Diff:

Revision 15 (+445 -51)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hxqlin
  1. 
      
  2. docs/rbtools/rbt/commands/setup-repo.rst (Diff revision 14)
     
     

    not really familiar with .rst but i'm not understanding how .. _configuring: turned into

    Usage
    $ rbt setup-repo [options]
    

    in the screenshot (captioned "After: setup-repo documentation") could you explain that?

    1. The usage section you're seeing is not defined by .. _configuring:. Rather, it's being defined by .. rbt-command-usage::, which is defined programmatically elsewhere in the codebase. I added .. _configuring: to define the steps I made in the documentation (if I recall correctly... I'll admit it's been some time since I've touched it).

  3. rbtools/commands/setup_repo.py (Diff revision 14)
     
     

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

  4. rbtools/commands/setup_repo.py (Diff revision 14)
     
     
     
     

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

  5. rbtools/commands/setup_repo.py (Diff revision 14)
     
     
     

    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 constants

  6. ?????

    1. This is a library that I'm using to spy on certain methods for testing.

  7. I think you can remove the if

  8. I'm not quite sure on the style rules for these tests but could we get some more whitespace in here if that's allowed? I feel like it's a bit hard to read when it's in a big block like this - same goes for the other tests

  9. same comment about removing if - same goes for below tests

  10. I think there's a typo in peforce - should be Perforce right? (not sure if the capitalization is needed but pretty sure there's an 'r' in there :P)

  11. rbtools/testing/transport.py (Diff revision 14)
     
     

    is this class providing something like a mock endpoint from which to retrieve repository URLs from? i'm not super clear on what it's doing. also, i don't think the methods execute_request_method and get_url are used anywhere

    1. In a way, yes. It's kinda like that. The type of information (a specific type of Resource instance) it returns is based on the type of payload it receives (as an argument). The parent class, Transport, usually reads the payload within the metadata of an HTTP request.

      I can try to write up a docstring that provides more info.

      execute_request_method is required in order for the mock to work. As for get_url, yeah I'm not sure if that's needed since the tests still work without it. I'll remove it for now.

  12. rbtools/testing/transport.py (Diff revision 14)
     
     

    Missing docstring

  13. 
      
bui1
  1. Just a couple minor things to address! Overall the content of the change is pretty solid :)

  2. rbtools/commands/setup_repo.py (Diff revision 14)
     
     

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

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

  4. Each of these docstrings needs a period at the end

    1. I don't think unit test docstrings need periods

      https://www.notion.so/reviewboard/Writing-Python-Unit-Tests-48924d31b6c0419bb40ec699d9cf4d90#ddd4c0e4b341424d9a2a83038707d5c6

  5. Could we seperate the contents of the unit test with a new line?

    That way it'd be like, where we setup our test, then call the function, then assert the results. It makes it easier to scan what's happening

    setup = self._create_setup_repo_command()
    self.spy_on(get_input, call_fake=self._mock_input)
    self.spy_on(confirm_select)
    self.spy_on(setup._display_rb_repositories)
    setup.default_transport_cls = TestTransport('testmockurl')
    mock_api_root = setup.default_transport_cls.get_root()
    
    output = setup.prompt_rb_repository('Git', RepositoryInfo(
        path='testpath'), mock_api_root)
    
    self.assertTrue(setup._display_rb_repositories.called)
    self.assertTrue(isinstance(output, ItemResource))
    
  6. could use self.assertNone instead

  7. could use self.assertTrue here

  8. Can use self.assertFalse here

  9. could use self.assertTrue here

  10. could use self.assertTrue here

  11. rbtools/utils/console.py (Diff revision 14)
     
     

    Rewording this line could help bring clarity more of that this function does.

    I think changing it to
    Accepted answers are integers starting from 1 until an integer n representing the nth element within options.
    should help

  12. 
      
kpatenio
kpatenio
kpatenio
david
  1. By and large the functionality here looks great. Most of these are pretty simple cleanup and style comments.

  2. docs/rbtools/rbt/commands/setup-repo.rst (Diff revision 18)
     
     

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

  3. docs/rbtools/rbt/commands/setup-repo.rst (Diff revision 18)
     
     
     

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

  4. docs/rbtools/rbt/commands/setup-repo.rst (Diff revision 18)
     
     
     
     

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

  5. docs/rbtools/rbt/commands/setup-repo.rst (Diff revision 18)
     
     

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

  6. rbtools/commands/__init__.py (Diff revision 18)
     
     

    Can we add a docstring for this please?

  7. rbtools/commands/setup_repo.py (Diff revision 18)
     
     

    This can go on the previous line.

  8. rbtools/commands/setup_repo.py (Diff revision 18)
     
     

    Please add a blank line above this one.

  9. rbtools/commands/setup_repo.py (Diff revision 18)
     
     

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

  10. rbtools/commands/setup_repo.py (Diff revision 18)
     
     

    I'm not pleased with all of these print statements which start with a newline. Perhaps we can add the extra newline to the end of the previous print statements? It would maybe result in an extra blank line at the end of the output, but that's probably fine.

    Alternatively, having two print statements is much prettier to look at:

    ```python
    print()
    print('No %s repository found for ...')

  11. rbtools/commands/setup_repo.py (Diff revision 18)
     
     

    I know other comments in here aren't all correct, but new ones should be in the imperative mood rather than passive ("Display all" instead of "Displays all")

  12. rbtools/commands/setup_repo.py (Diff revision 18)
     
     

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

  13. rbtools/commands/setup_repo.py (Diff revision 18)
     
     
     

    Indent this 4 more spaces.

  14. rbtools/commands/setup_repo.py (Diff revision 18)
     
     

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

  15. rbtools/commands/setup_repo.py (Diff revision 18)
     
     

    Indent this four more spaces.

  16. Instead of defining self._mock_input, you should be able to just use:

    self.spy_on(get_input, call_fake=lambda: '1')
    
  17. rbtools/commands/tests/test_setup_repo.py (Diff revision 18)
     
     
     

    The way this is wrapped is confusing (it wraps in the middle of inner parens, making it unclear that path= is an arg to RepositoryInfo instead of prompt_rb_repository). How about:

    output = setup.prompt_rb_repository(
        'Git', RepositoryInfo(path='testpath'), mock_api_root)
    

    You could also put each arg on its own line if you like.

  18. rbtools/commands/tests/test_setup_repo.py (Diff revision 18)
     
     
     

    Same here re: wrapping.

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

  20. rbtools/testing/transport.py (Diff revision 18)
     
     

    Please add a module docstring for this file.

  21. rbtools/testing/transport.py (Diff revision 18)
     
     

    This needs a docstring.

  22. rbtools/testing/transport.py (Diff revision 18)
     
     
     
     
     

    Wrapping is a little funky here.

  23. rbtools/testing/transport.py (Diff revision 18)
     
     
     

    Indent this 4 more spaces.

  24. rbtools/testing/transport.py (Diff revision 18)
     
     

    This should include a "Returns:" section.

  25. rbtools/testing/transport.py (Diff revision 18)
     
     

    This should include a "Returns:" section.

  26. rbtools/utils/console.py (Diff revision 18)
     
     

    The python type is named "int"

  27. rbtools/utils/console.py (Diff revision 18)
     
     
     

    Please add a blank line between these two.

  28. rbtools/utils/console.py (Diff revision 18)
     
     

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

  29. rbtools/utils/console.py (Diff revision 18)
     
     

    Please add a blank line above this.

  30. 
      
kpatenio
Review request changed

Change Summary:

Addressed David's review

Testing Done:

   

Test cases run on python 2.7.17 and 3.7.6:

    - Test case validating repository prompt if repositories exist
    - Test case validating repository prompt if no repositories exist
    - Test case validating config file generation
    - Test case validating the contents of config file
    - Test case validating server options
~   - Test case validating peforce options
  ~ - Test case validating perforce options
    - Test case validating tfs options

Commit:

-b9f8adbacbc18d9c314705e40b935e82bef57bb8
+26e0420e5252081f84f20f2af8b58084f59f3668

Diff:

Revision 19 (+486 -51)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

kpatenio
mike_conley
  1. This looks great, Katherine! Just a few cosmetic notes and questions.

  2. docs/rbtools/rbt/commands/setup-repo.rst (Diff revision 20)
     
     
     
     

    I think we should probably make it more clear that this first sentence is only for RBCommons subscribers. For folks doing their own deployments (who might not know what RBCommons is), this might be a little confusing.

    Perhaps:

    If you are an RBCommons customer, you can add repositories under your team administration settings. If you are managing your own deployment of Review Board, refer to the Admin Dashboard. See :ref:`here <rb:repositories>` for more information.
    
  3. rbtools/commands/setup_repo.py (Diff revision 20)
     
     

    Looking around at the other commands, I think we tend to use print() in place of inserting \n characters. Example: https://github.com/reviewboard/rbtools/blob/85534a29cf9d7e7b0defdf1ba6817dc6c71491e3/rbtools/commands/info.py#L62

  4. rbtools/commands/setup_repo.py (Diff revision 20)
     
     

    Same as above, re: \n characters.

  5. rbtools/commands/setup_repo.py (Diff revision 20)
     
     
     
     
     
     
     
     
     
     

    Same as above, re: \n characters.

    1. I updated this particular section here to use print(). Let me know if it looks good!

  6. rbtools/commands/setup_repo.py (Diff revision 20)
     
     
     
     
     
     
     
     
     
     

    If possible, let's try to be consistent about where we put spaces at line breaks. I believe the convention in rbtools is to put the space at the end of the line rather than the beginning.

  7. rbtools/commands/setup_repo.py (Diff revision 20)
     
     

    Same as above, re: \n characters.

  8. rbtools/commands/setup_repo.py (Diff revision 20)
     
     

    Same as above, re: \n characters.

  9. rbtools/commands/setup_repo.py (Diff revision 20)
     
     

    Same as above, re: \n characters.

  10. rbtools/commands/setup_repo.py (Diff revision 20)
     
     

    Same as above, re: \n characters.

  11. rbtools/commands/setup_repo.py (Diff revision 20)
     
     
     

    Same as above, re: \n characters.

    1. I'm not sure where to fit in print() for this specific line, since question will be a string
      passed into confirm(). Do you have an example?

    2. Ah, I'd missed that. Thanks, dropping issue. :)

  12. rbtools/commands/setup_repo.py (Diff revision 20)
     
     
     

    So if they say no, do we just ask again? Remind me again - was it a conscious choice on our part not to exit if they said "no"?

    1. Hi Mike! Thanks for your review - apologies for the delayed response!

      In the current version of setup-repo, choosing no for a respository we select in the prompt will close the command.
      I wanted to change this since it can be inconvenient to rereun the command agsin if we change our mind about a repository.

      That said, if you and other mentors advise against this change, I'd be happy to revert/modify it. :)

    2. Ah, I'd missed that too, thanks for clearing that up. :)

  13. 
      
kpatenio
kpatenio
mike_conley
  1. I'm really happy with this - great job, Katherine! I just have one last question related to how some of these print statements are formatted.

  2. rbtools/commands/setup_repo.py (Diff revision 22)
     
     
     

    Looking through some of the other RBTools files, I think the convention for formatting here is:

    printf('Some formatted string: %s, %s, %s'
           % (list, of, variables))
    

    Or was the linter bugging you about this?

    1. To clarify, are you referring to %i?

    2. Yep!
    3. Christian suggested in his review to use %d instead of %i. So that may help us determine what to use. :D

  3. 
      
chipx86
  1. Hey, this is great!!

    Don't be scared by the number of comments here. It's mostly tiny syntactical nits, and I repeat them where I see them. You'll be able to knock them off quickly.

    Most of this is actually doc-related, just trying to get the docs to conform to our standards a bit better (using ReStructuredText role indicators to reference classes/methods/attributes/modules, pointing out missing docs, tweaks to wording, etc.).

    The core of this is great and I cannot wait to get this in (I just used the current setup-repo last night for a new repository and immediately wished I was running this code).

  2. docs/rbtools/rbt/commands/setup-repo.rst (Diff revision 22)
     
     
     
     

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

    We do collapse that down if we have multiple directives in a row, but when we're in a situation like this, it helps to distinguish the directive from an anchor (the .. _blah:).

  3. docs/rbtools/rbt/commands/setup-repo.rst (Diff revision 22)
     
     

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

    The reference can be at the bottom of the file:

    .. _RBCommons: https://rbcommons.com/
    
  4. docs/rbtools/rbt/commands/setup-repo.rst (Diff revision 22)
     
     

    "here" is almost always a bad choice for a label for an anchor, because screen readers can't really present that in any meaningful way.

    Instead, let's do:

    :ref:`Learn more about configuring repositories <rb:repositories>`.
    
  5. docs/rbtools/rbt/commands/setup-repo.rst (Diff revision 22)
     
     

    We can use :ref:rbtools-reviewboardrc to reference .reviewboardrc. It will have the right name, and it will link to the documentation on the file. (This is fairly new.)

    Let's also reference the REVIEWBOARD_URL item directly. You'd need to create an anchor for it in rbt/configuration/repositories.rst, but it'll help.

  6. docs/rbtools/rbt/commands/setup-repo.rst (Diff revision 22)
     
     
     

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

  7. docs/rbtools/rbt/commands/setup-repo.rst (Diff revision 22)
     
     
     
     

    Let's do two blank lines here.

  8. rbtools/api/tests/base.py (Diff revision 22)
     
     
     
     

    Keys should all be in alphabetical order.

    Same with ones below.

  9. rbtools/api/tests/base.py (Diff revision 22)
     
     

    This is missing a trailing comma.

  10. rbtools/api/tests/base.py (Diff revision 22)
     
     

    This is missing a trailing comma.

  11. rbtools/api/tests/base.py (Diff revision 22)
     
     
     

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

  12. rbtools/api/tests/base.py (Diff revision 22)
     
     

    This should be an absolute path.

  13. rbtools/api/tests/base.py (Diff revision 22)
     
     

    Missing a trailing comma.

  14. rbtools/commands/__init__.py (Diff revision 22)
     
     
     
     
     
     
     
     
     
     
     

    There's a tweak to this that I'd like to see. It was what I had in my head when we first discussed this on Slack, but I'm not sure I covered the use case well enough.

    In your current implementation, default_transport_cls is assigned during initialization. What I had intended was for this to be a class-level attribute, like:

    default_transport_cls = SyncTransport
    

    (Documented using our standard class attribute documenting conventions, using #: syntax.)

    That's the default one for the class, which can be overridden during initialization.

    __init__ would take None as its default value. The body would then be:

    self.transport_cls = transport_cls or self.default_transport_cls
    

    This gives us some advantages:

    1. A command can easily customize its default transport, and subclasses can benefit from that default as well, without mucking about with passing things in __init__.
    2. The transport for a class can be introspected or even replaced (during testing) without first instantiating the class (useful for some categories of tests).
    3. When initializing, we can still override for that specific instance.
    1. I didn't even know we could do such thing in Python! Mindblown :O

  15. rbtools/commands/__init__.py (Diff revision 22)
     
     

    This would be a little more clear as:

    """Initialize the base functionality for the command.
    
  16. rbtools/commands/__init__.py (Diff revision 22)
     
     

    This must use the absolute path to the type, so rbtools.api.transport.Transport.

    It must also have , optional after the type, since it's not required.

  17. rbtools/commands/__init__.py (Diff revision 22)
     
     
     

    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`.
    

    The current description is describing mostly what happens to this. We want to focus more on what this is for.

  18. rbtools/commands/__init__.py (Diff revision 22)
     
     

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

  19. rbtools/commands/setup_repo.py (Diff revision 22)
     
     
     
     

    The % should be on the last line.

  20. rbtools/commands/setup_repo.py (Diff revision 22)
     
     
     
     
     
     

    We're eventually going to want to localize our code, and we can get a head start by using a different format for placeholders in format strings:

    print(
        '%(num)s %(repo_type)s repositories found:'
        % {
            'num': len(closest_paths),
            'repo_type': tool_name,
        })
    

    The reason for this is that, when you localize, you may end up with a sentence that requires changes in the order of replaced values. So maybe the number comes after the type name. We can't make this happen if we're using just %s or similar, but we can if they're named.

    Same sort of thing in strings below.

  21. rbtools/commands/setup_repo.py (Diff revision 22)
     
     
     

    Blank line between these.

  22. rbtools/commands/setup_repo.py (Diff revision 22)
     
     
     

    Blank line here (the return is the conclusion, and not part of the outputting block of code, so it gets its own paragraph).

  23. rbtools/commands/setup_repo.py (Diff revision 22)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    There's a lot of strings like these that are going to be pretty long, and they might not look good on all terminals. Terminals will wrap at the boundary, even if it breaks a word, so we need to do better.

    Long-term plan is to replace print() with something that will handle this for us, but short-term, you can do:

    print(textwrap.fill('.....'))
    

    This will wrap at 70 characters, which is safe for most terminals, and will be safe for future localization.

    1. Interesting, I've never seen this before. To clarify, print(textwrap.fill('.....')) will replace all print statements here with only one, where we will wrap the entire long string?

      Does this mean we'll have to use \n in place of print()?

    2. We'll need one per paragraph. It doesn't handle multi-paragraph strings. So it'll be just like what you have right now, but with the text wrapping. Whitespace within a string is condensed and leading/trailing whitespace is trimmed.

      It's messy, but we have plans to unify how output is done in commands in a future change.

  24. rbtools/commands/setup_repo.py (Diff revision 22)
     
     

    The % CONFIG_FILE should go on its own line.

  25. rbtools/commands/setup_repo.py (Diff revision 22)
     
     

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

  26. rbtools/commands/setup_repo.py (Diff revision 22)
     
     

    "URL"

    "neither api_root nor ..."

  27. rbtools/commands/setup_repo.py (Diff revision 22)
     
     

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

  28. rbtools/commands/setup_repo.py (Diff revision 22)
     
     

    You'll need to test this part a bit more, because it will break. When passing multiple values to %, you need to provide a tuple. What you're actually providing here is 1 formatting value, and a new argument to print().

  29. rbtools/commands/setup_repo.py (Diff revision 22)
     
     

    Can you use backticks around the rbt --help setup-repo?

    Maybe even just do rbt help setup-repo, to keep it a little cleaner.

  30. rbtools/commands/setup_repo.py (Diff revision 22)
     
     
     

    This should be formatted like:

    question = (
        '.......'
        % (outfile_path, output)
    )
    
  31. rbtools/commands/setup_repo.py (Diff revision 22)
     
     
     
     
     
     

    When you have code that needs to iterate through something and return both an index and a value, it's best to use enumerate, like so:

    for i, repo_name in enumerate(closest_paths):
        print(....)
    
  32. rbtools/commands/setup_repo.py (Diff revision 22)
     
     

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

  33. rbtools/commands/setup_repo.py (Diff revision 22)
     
     
     

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

  34. For os-level commands, we prefer to import os and then reference as os.path, etc.

    This avoids common problems with redefining something like path, and provides contexts to the calls.

  35. rbtools/commands/tests/test_setup_repo.py (Diff revision 22)
     
     
     
     
     
     
     

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

  36. rbtools/commands/tests/test_setup_repo.py (Diff revision 22)
     
     
     
     
     
     

    It's best to group things into paragraphs, to avoid code just all blurring together.

    Spying on functions is a paragraph. We can describe it as something distinct from setting up the command, or setting up state, or invoking behavior, or asserting results. These are all paragraphs.

    You can apply those to tests below.

  37. rbtools/commands/tests/test_setup_repo.py (Diff revision 22)
     
     
     

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

    Same below.

  38. You can use assertIsInstance.

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

  40. rbtools/commands/tests/test_setup_repo.py (Diff revision 22)
     
     
     
     

    It's best to format these things like:

    self.assertEqual(
        config_lines,
        [
            '...',
            '...',
            '...',
        ])
    

    Helps break them apart, keep them maintainable. Easier to add new items without worrying about re-wrapping later.

  41. Missing a trailing comma.

  42. rbtools/commands/tests/test_setup_repo.py (Diff revision 22)
     
     
     
     
     
     
     
     
     
     
     

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

    Same with other tests.

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

    This isn't performance-critical code, so it doesn't actually matter, but we use it for consistency now.

  44. rbtools/testing/transport.py (Diff revision 22)
     
     
     

    Blank line between class docstrings and members.

  45. rbtools/testing/transport.py (Diff revision 22)
     
     

    Missing docs for *args and **kwargs.

  46. rbtools/testing/transport.py (Diff revision 22)
     
     

    Missing , optional.

  47. rbtools/testing/transport.py (Diff revision 22)
     
     
     

    When referencing classes, we use:

    :py:class:`absolute.path.to.Class`
    

    For modules, use :py:mod:.

  48. rbtools/testing/transport.py (Diff revision 22)
     
     

    Missing , optional.

  49. rbtools/testing/transport.py (Diff revision 22)
     
     
     

    Same notes as above.

  50. rbtools/testing/transport.py (Diff revision 22)
     
     

    We shouldn't use backticks in the summary. It can turn out wonky.

    We also really want to describe here what the function does, not that it's a mock. That's a "what is it" doc, not a "what does it do".

  51. rbtools/testing/transport.py (Diff revision 22)
     
     
     

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

  52. rbtools/testing/transport.py (Diff revision 22)
     
     

    When referencing an attribute on the class, use:

    :py:attr:`list_payload`
    
  53. rbtools/testing/transport.py (Diff revision 22)
     
     

    Missing docs for *args and **kwargs.

  54. rbtools/testing/transport.py (Diff revision 22)
     
     

    Should use :py:class:.

  55. rbtools/testing/transport.py (Diff revision 22)
     
     

    This must be an absolute class path.

  56. rbtools/testing/transport.py (Diff revision 22)
     
     

    API requests always use absolute URLs.

  57. rbtools/testing/transport.py (Diff revision 22)
     
     
     

    Blank line between these.

  58. rbtools/testing/transport.py (Diff revision 22)
     
     

    Same comments as above.

    Also, there's some leading whitespace here.

  59. rbtools/testing/transport.py (Diff revision 22)
     
     

    Use :py:meth: to reference methods.

    Same comments otherwise apply to these docs.

  60. rbtools/testing/transport.py (Diff revision 22)
     
     

    This should be an absolute path.

  61. rbtools/utils/console.py (Diff revision 22)
     
     
     

    This can be one statement.

  62. rbtools/utils/console.py (Diff revision 22)
     
     
     
     

    No blank line here.

  63. 
      
kpatenio
kpatenio
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

kpatenio
david
  1. Ship It!
  2. 
      
kpatenio
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (73f013b)
Loading...