Make rbt setup-repo more clear

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

kpatenio
RBTools
release-2.0.x
928aff8...
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 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

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
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
Review request changed

Change Summary:

Updated descriptions to be shorter + setup-repo docs + minor comment styling

Commit:

-b224dcb6a8343894f3474c7f1dfecd00e0c78b0b
+928aff8ea165c0033cb3e9275e692d04a7bce622

Diff:

Revision 12 (+307 -48)

Show changes

Removed Files:

Added Files:

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...