flake8
-
rbtools/commands/__init__.py (Diff revision 1) -
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-hints
is 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: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+36 -2) |
rbtools/commands/__init__.py (Diff revision 2) |
---|
E122 continuation line missing indentation or outdented
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+36 -2) |
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+68 -2) |
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+70 -2) |
Testing Done: |
|
---|
Testing Done: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Groups: |
|
Testing Done: |
|
---|
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+70 -2) |
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+70 -2) |
docs/rbtools/rbt/configuration/users.rst (Diff revision 7) |
---|
Let's say "RBTools commands may print helpful hints"
docs/rbtools/rbt/configuration/users.rst (Diff revision 7) |
---|
"be provided" suggests to me that one could specify a value. Perhaps say "Hints can also be disabled by passing ..."?
rbtools/commands/__init__.py (Diff revision 7) |
---|
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):
rbtools/utils/commands.py (Diff revision 7) |
---|
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: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+84 -12) |
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+86 -12) |
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+86 -12) |
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+88 -12) |
rbtools/commands/__init__.py (Diff revision 11) |
---|
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: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 12 (+128 -12) |
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 13 (+128 -12) |
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_hint
method on theCommand
class 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
.reviewboardrc
is part of your change, but it shouldn't be. Can you revert that part of your change?
rbtools/commands/__init__.py (Diff revision 13) |
---|
This is wrapping way prematurely. The docs should have room to breathe. The lines get to go up to 79 characters.
rbtools/commands/__init__.py (Diff revision 13) |
---|
Blank line between statements and blocks, or between two separate blocks.
Commits: |
|
||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 14 (+181 -57) |
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".
.reviewboardrc (Diff revision 14) |
---|
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.
rbtools/commands/__init__.py (Diff revision 14) |
---|
Typo: "will speed up will speed up" -> "will speed up"
Description: |
|
|||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||
Commits: |
|
|||||||||||||||||||||
Diff: |
Revision 15 (+126 -10) |