Adding helpful hints for rbt commands
Review Request #11405 — Created Jan. 24, 2021 and discarded
RBTools currently must scan the filesystem when a user does not specify a
repository name or repository type which is very slow. In order to optimize
RBTools, we will log hints that recommends setting repository name or type
when it has not been specified. An option--no-hintsis added so
users can disable hints if desired.
I ran
./tests/runtests.py rbtools/and have 263 tests passing and have done
manual testing on different rbtools commands to ensure that the--no-hints
option is working and is also presented as an option to the user when they
typerbt [command] --help.
| Summary | ID | Author |
|---|---|---|
| c186fdb4eae575704451e84f3063309628bb8f17 | anahita-m |
| Description | From | Last Updated |
|---|---|---|
|
This looks pretty good, but I have a thought. We have these hints right now, but we may want more … |
 chipx86
|
|
|
Looks like .reviewboardrc is part of your change, but it shouldn't be. Can you revert that part of your change? |
chipx86
|
|
|
Typo in the description: "RBTools current" -> "RBTools currently". Also in the description, "when it already has not been specified." … |
 mike_conley
|
|
|
E501 line too long (93 > 79 characters) |
 reviewbot
|
|
|
E501 line too long (89 > 79 characters) |
reviewbot
|
|
|
E122 continuation line missing indentation or outdented |
reviewbot
|
|
|
E502 the backslash is redundant between brackets |
reviewbot
|
|
|
This should be "True" instead of "TRUE" |
 david
|
|
|
Let's say "RBTools commands may print helpful hints" |
david
|
|
|
"be provided" suggests to me that one could specify a value. Perhaps say "Hints can also be disabled by passing … |
david
|
|
|
Setting the repository type and name each enable different optimizations, and for best results users should have both in their … |
david
|
|
|
This was already mixed up, but can we alphabetize this by key while we're editing it? That'll make it easier … |
david
|
|
|
Space at the end of the string after the . |
 ryankang
|
|
|
These log messages don't make it entirely clear why the user should set these or what the consequences are of … |
david
|
|
|
E501 line too long (83 > 79 characters) |
reviewbot
|
|
|
This is wrapping way prematurely. The docs should have room to breathe. The lines get to go up to 79 … |
chipx86
|
|
|
Blank line between statements and blocks, or between two separate blocks. |
chipx86
|
|
|
Christian's comment still applies - this .reviewboardrc file shouldn't be in here. See https://github.com/k88hudson/git-flight-rules#i-want-to-remove-a-file-from-the-previous-commit on how to clean that up. |
mike_conley
|
|
|
Typo: -no-hints -> --no-hints. |
mike_conley
|
|
|
Typo: "will speed up will speed up" -> "will speed up" |
mike_conley
|
- Commits:
-
Summary ID Author 695f5a8911c1f124721f0b317328eda12b25942c anahita-m c1c7bafee8df1144f518bb923da571dd5ef00af7 anahita-m - Diff:
-
Revision 2 (+36 -2)
- Commits:
-
Summary ID Author c1c7bafee8df1144f518bb923da571dd5ef00af7 anahita-m c619b59efb49789400f0498a66a242c1f984f851 anahita-m - Diff:
-
Revision 3 (+36 -2)
Checks run (2 succeeded)
- Commits:
-
Summary ID Author c619b59efb49789400f0498a66a242c1f984f851 anahita-m 945545ec72d3239246e237e4ac2d7f8e2333da81 anahita-m
Checks run (2 succeeded)
- Commits:
-
Summary ID Author 945545ec72d3239246e237e4ac2d7f8e2333da81 anahita-m 179b12c155e0ae001061f54c7cb189dff0297bef anahita-m
Checks run (2 succeeded)
- Testing Done:
-
+ I ran
./tests/runtests.py rbtools/and have 263 tests passing and have done+ manual testing on different rbtools commands to ensure that the --no-hints + option is working and is also presented as a global option to the user when + they type rbt [command] --help.
- Testing Done:
-
~ I ran
./tests/runtests.py rbtools/and have 263 tests passing and have done~ manual testing on different rbtools commands to ensure that the --no-hints ~ I ran
./tests/runtests.py rbtools/and have 263 tests passing and have donemanual testing on different rbtools commands to ensure that the --no-hints option is working and is also presented as an option~ to the user when they type rbt [command] --help.- option is working and is also presented as a global option to the user when - they type rbt [command] --help. - Groups:
- Testing Done:
-
~ I ran
./tests/runtests.py rbtools/and have 263 tests passing and have donemanual testing on different rbtools commands to ensure that the --no-hints option is working and is also presented as an option~ to the user when they type rbt [command] --help.~ I ran
./tests/runtests.py rbtools/and have 263 tests passing and have done~ manual testing on different rbtools commands to ensure that the --no-hints + option is working and is also presented as an option to the user when they + type rbt [command] --help.
- Commits:
-
Summary ID Author 179b12c155e0ae001061f54c7cb189dff0297bef anahita-m a785c2f723e13218fd6c1ca66bfc2d59db5c09b3 anahita-m
- Commits:
-
Summary ID Author a785c2f723e13218fd6c1ca66bfc2d59db5c09b3 anahita-m 1f4c6f892eb5d06f856a6c1a75e16a980b60e65a anahita-m
Checks run (2 succeeded)
-
-
-
-
"be provided" suggests to me that one could specify a value. Perhaps say "Hints can also be disabled by passing ..."?
-
Setting the repository type and name each enable different optimizations, and for best results users should have both in their config file. We should probably have separate hints.
Also, FYI, when we're wrapping a conditional like this we prefer using parentheses instead of the continuation character:
if (self.options.repository_name is None and self.options.repository_type is None): -
This was already mixed up, but can we alphabetize this by key while we're editing it? That'll make it easier to read.
- Commits:
-
Summary ID Author 1f4c6f892eb5d06f856a6c1a75e16a980b60e65a anahita-m 0e4a78b3b7059a526ca5d9d4bd7627ed3a8ce7c5 anahita-m
Checks run (2 succeeded)
- Commits:
-
Summary ID Author 0e4a78b3b7059a526ca5d9d4bd7627ed3a8ce7c5 anahita-m e7a21e9305e0b3ebfce4a7a0e44054747dbd71db anahita-m
Checks run (2 succeeded)
- Commits:
-
Summary ID Author e7a21e9305e0b3ebfce4a7a0e44054747dbd71db anahita-m 936ccdc624a60d7a8d75daf0db53dc25bed2b6dc anahita-m
Checks run (2 succeeded)
- Commits:
-
Summary ID Author 936ccdc624a60d7a8d75daf0db53dc25bed2b6dc anahita-m 2e8c116e97939f101887bae421b622c3ef27dff1 anahita-m
Checks run (2 succeeded)
-
-
These log messages don't make it entirely clear why the user should set these or what the consequences are of not doing so. How about something like:
RBTools is auto-detecting the repository type, which can be slow. Adding REPOSITORY_TYPE to your .reviewboardrc will speed up rbt commands. Set ENABLE_HINTS=False to suppress this message.
RBTools is attempting to discover the matching repository in the Review Board server. Adding REPOSITORY_NAME to your .reviewboardrc will speed up rbt commands. Set ENABLE_HINTS=False to suppress this message.
Looking at those, I'm reconsidering my suggesting to combine them into a single message, but maybe there's a good way to word a single message without it being too excessively long.
- Commits:
-
Summary ID Author 2e8c116e97939f101887bae421b622c3ef27dff1 anahita-m f7272c460fe0ab71b61f5c46d0bf704f59a5e239 anahita-m
- Commits:
-
Summary ID Author f7272c460fe0ab71b61f5c46d0bf704f59a5e239 anahita-m 63e8bbcf704b383e8c9b6cf7d83a0c21f12ab372 anahita-m
Checks run (2 succeeded)
-
-
This looks pretty good, but I have a thought. We have these hints right now, but we may want more in the future, and we probably don't want each hint to decide what the output looks like (e.g., the info on suppressing hints being tacked onto every string).
How about defining a new
show_hintmethod on theCommandclass that takes in a string, checks if hints are enabled, and conditionally outputs it.That would clean up the code using it now, and make it very easy to start adding new hints down the road.
-
Looks like
.reviewboardrcis part of your change, but it shouldn't be. Can you revert that part of your change? -
This is wrapping way prematurely. The docs should have room to breathe. The lines get to go up to 79 characters.
-
- Commits:
-
Summary ID Author 63e8bbcf704b383e8c9b6cf7d83a0c21f12ab372 anahita-m 63e8bbcf704b383e8c9b6cf7d83a0c21f12ab372 anahita-m 670a17e0d9dff492f71fda82e76ca710c9d6a76e anahita-m 2871e852c6cb827f46ab2e5dcc144897cd877ead anahita-m
Checks run (2 succeeded)
-
Thanks, Anahita! Just a few comments below.
-
Typo in the description: "RBTools current" -> "RBTools currently".
Also in the description, "when it already has not been specified." is kind of awkward. Maybe this can just be "when it has not been specified".
-
Christian's comment still applies - this
.reviewboardrcfile shouldn't be in here. See https://github.com/k88hudson/git-flight-rules#i-want-to-remove-a-file-from-the-previous-commit on how to clean that up. -
-
- Description:
-
~ RBTools current must scan the filesystem when a user does not specify a
~ RBTools currently must scan the filesystem when a user does not specify a
repository name or repository type which is very slow. In order to optimize RBTools, we will log hints that recommends setting repository name or type ~ when it already has not been specified. An option --no-hints is added so ~ when it has not been specified. An option --no-hintsis added sousers can disable hints if desired. - Testing Done:
-
I ran
./tests/runtests.py rbtools/and have 263 tests passing and have done~ manual testing on different rbtools commands to ensure that the --no-hints ~ manual testing on different rbtools commands to ensure that the --no-hintsoption is working and is also presented as an option to the user when they type rbt [command] --help. - Commits:
-
Summary ID Author 63e8bbcf704b383e8c9b6cf7d83a0c21f12ab372 anahita-m 670a17e0d9dff492f71fda82e76ca710c9d6a76e anahita-m 2871e852c6cb827f46ab2e5dcc144897cd877ead anahita-m c186fdb4eae575704451e84f3063309628bb8f17 anahita-m