Make rbt setup-repo more clear

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

kpatenio
RBTools
release-2.0.x
1c16e3d...
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
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
Review request changed

Change Summary:

Fixed minor linting issue that I forgot to properly address

Commit:

-26e0420e5252081f84f20f2af8b58084f59f3668
+1c16e3dbda24a271f944d7a0b45c356b960c8790

Diff:

Revision 20 (+487 -51)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
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.

  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.

  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"?

  13. 
      
Loading...