- Download Diff

Summary:

Adding helpful hints for rbt commands

Review Request #11405 — Created Jan. 24, 2021 and discarded Aug. 27, 2021, 3:37 p.m.

Information
Owner:	amohapatra
Repository:	RBTools
Branch:	master
Bugs:
Depends On:
Reviewers
Groups:	rbtools, students
People:

Description

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.

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 an option to the user when they
type rbt [command] --help.

Commits

Summary	Author
reverting .reviewboardrc change	anahita-m

Screenshots

Issues

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	April 4, 2021, 5:14 p.m.
Looks like .reviewboardrc is part of your change, but it shouldn't be. Can you revert that part of your change?	chipx86	April 4, 2021, 9:29 p.m.
Typo in the description: "RBTools current" -> "RBTools currently". Also in the description, "when it already has not been specified." ...	mike_conley	April 4, 2021, 9:29 p.m.
E501 line too long (93 > 79 characters)	reviewbot	Jan. 24, 2021, 10:27 p.m.
E501 line too long (89 > 79 characters)	reviewbot	Jan. 24, 2021, 10:27 p.m.
E122 continuation line missing indentation or outdented	reviewbot	Jan. 24, 2021, 10:31 p.m.
E502 the backslash is redundant between brackets	reviewbot	Feb. 6, 2021, 7:39 a.m.
This should be "True" instead of "TRUE"	david	Feb. 8, 2021, 9:07 p.m.
Let's say "RBTools commands may print helpful hints"	david	Feb. 8, 2021, 9:07 p.m.
"be provided" suggests to me that one could specify a value. Perhaps say "Hints can also be disabled by passing ...	david	Feb. 8, 2021, 9:06 p.m.
Setting the repository type and name each enable different optimizations, and for best results users should have both in their ...	david	Feb. 8, 2021, 9:09 p.m.
This was already mixed up, but can we alphabetize this by key while we're editing it? That'll make it easier ...	david	Feb. 8, 2021, 9:06 p.m.
Space at the end of the string after the .	ryankang	Feb. 13, 2021, 3:53 p.m.
These log messages don't make it entirely clear why the user should set these or what the consequences are of ...	david	Feb. 28, 2021, 12:29 p.m.
E501 line too long (83 > 79 characters)	reviewbot	Feb. 28, 2021, 12:29 p.m.
This is wrapping way prematurely. The docs should have room to breathe. The lines get to go up to 79 ...	chipx86	April 4, 2021, 12:56 a.m.
Blank line between statements and blocks, or between two separate blocks.	chipx86	April 4, 2021, 12:57 a.m.
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	April 4, 2021, 9:29 p.m.
Typo: -no-hints -> --no-hints.	mike_conley	April 4, 2021, 8:29 p.m.
Typo: "will speed up will speed up" -> "will speed up"	mike_conley	April 4, 2021, 8:29 p.m.

flake8 failed.

JSHint passed.

flake8

rbtools/commands/__init__.py (Diff revision 1)
Show all issues
```
E501 line too long (93 > 79 characters)
```
rbtools/commands/__init__.py (Diff revision 1)
Show all issues
```
E501 line too long (89 > 79 characters)
```

Commits:

	Summary		Author
-		[WIP] Adding helpful hints for rbt commands	anahita-m
+		[WIP] Adding helpful hints for rbt commands	anahita-m

Diff:

Revision 2 (+36 -2)

Show changes

	.reviewboardrc
	rbtools/commands/__init__.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/__init__.py (Diff revision 2)

Show all issues

E122 continuation line missing indentation or outdented

Commits:

	Summary		Author
-		[WIP] Adding helpful hints for rbt commands	anahita-m
+		[WIP] Adding helpful hints for rbt commands	anahita-m

Diff:

Revision 3 (+36 -2)

Show changes

	.reviewboardrc
	rbtools/commands/__init__.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commits:

	Summary		Author
-		[WIP] Adding helpful hints for rbt commands	anahita-m
+		[WIP] Adding helpful hints for rbt commands	anahita-m

Diff:

Revision 4 (+68 -2)

Show changes

	.reviewboardrc
	docs/rbtools/rbt/configuration/users.rst
	rbtools/commands/__init__.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commits:

	Summary		Author
-		[WIP] Adding helpful hints for rbt commands	anahita-m
+		[WIP] Adding helpful hints for rbt commands	anahita-m

Diff:

Revision 5 (+70 -2)

Show changes

	.reviewboardrc
	docs/rbtools/rbt/configuration/users.rst
	rbtools/commands/__init__.py
	rbtools/utils/commands.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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:

+	students

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		Author
-		[WIP] Adding helpful hints for rbt commands	anahita-m
+		[WIP] Adding helpful hints for rbt commands	anahita-m

Diff:

Revision 6 (+70 -2)

Show changes

	.reviewboardrc
	docs/rbtools/rbt/configuration/users.rst
	rbtools/commands/__init__.py
	rbtools/utils/commands.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/__init__.py (Diff revision 6)
Show all issues
```
E502 the backslash is redundant between brackets
```

Commits:

	Summary		Author
-		[WIP] Adding helpful hints for rbt commands	anahita-m
+		[WIP] Adding helpful hints for rbt commands	anahita-m

Diff:

Revision 7 (+70 -2)

Show changes

	.reviewboardrc
	docs/rbtools/rbt/configuration/users.rst
	rbtools/commands/__init__.py
	rbtools/utils/commands.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

docs/rbtools/rbt/configuration/users.rst (Diff revision 7)
Show all issues
```
This should be "True" instead of "TRUE"
```
docs/rbtools/rbt/configuration/users.rst (Diff revision 7)
Show all issues
```
Let's say "RBTools commands may print helpful hints"
```

docs/rbtools/rbt/configuration/users.rst (Diff revision 7)

Show all issues

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

Show all issues

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)

Show all issues

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		Author
-		[WIP] Adding helpful hints for rbt commands	anahita-m
+		[WIP] Adding helpful hints for rbt commands	anahita-m

Diff:

Revision 8 (+84 -12)

Show changes

	.reviewboardrc
	docs/rbtools/rbt/configuration/users.rst
	rbtools/commands/__init__.py
	rbtools/utils/commands.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commits:

	Summary		Author
-		[WIP] Adding helpful hints for rbt commands	anahita-m
+		[WIP] Adding helpful hints for rbt commands	anahita-m

Diff:

Revision 9 (+86 -12)

Show changes

	.reviewboardrc
	docs/rbtools/rbt/configuration/users.rst
	rbtools/commands/__init__.py
	rbtools/utils/commands.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/commands/__init__.py (Diff revision 9)
Show all issues
```
Space at the end of the string after the .
```

Commits:

	Summary		Author
-		[WIP] Adding helpful hints for rbt commands	anahita-m
+		Adding helpful hints for rbt commands	anahita-m

Diff:

Revision 10 (+86 -12)

Show changes

	.reviewboardrc
	docs/rbtools/rbt/configuration/users.rst
	rbtools/commands/__init__.py
	rbtools/utils/commands.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Summary:

-	[WIP] Adding helpful hints for rbt commands
+	Adding helpful hints for rbt commands

Commits:

	Summary		Author
-		Adding helpful hints for rbt commands	anahita-m
+		Adding helpful hints for rbt commands	anahita-m

Diff:

Revision 11 (+88 -12)

Show changes

	.reviewboardrc
	docs/rbtools/rbt/configuration/users.rst
	rbtools/commands/__init__.py
	rbtools/utils/commands.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/commands/__init__.py (Diff revision 11)

Show all issues

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		Author
-		Adding helpful hints for rbt commands	anahita-m
+		Adding helpful hints for rbt commands	anahita-m

Diff:

Revision 12 (+128 -12)

Show changes

	.reviewboardrc
	docs/rbtools/rbt/configuration/users.rst
	rbtools/commands/__init__.py
	rbtools/utils/commands.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/__init__.py (Diff revision 12)
Show all issues
```
E501 line too long (83 > 79 characters)
```

Commits:

	Summary		Author
-		Adding helpful hints for rbt commands	anahita-m
+		Adding helpful hints for rbt commands	anahita-m

Diff:

Revision 13 (+128 -12)

Show changes

	.reviewboardrc
	docs/rbtools/rbt/configuration/users.rst
	rbtools/commands/__init__.py
	rbtools/utils/commands.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Show all issues

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

Show all issues

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)

Show all issues

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)

Show all issues

Blank line between statements and blocks, or between two separate blocks.

Commits:

		Author
-	Adding helpful hints for rbt commands	anahita-m
+	Adding helpful hints for rbt commands	anahita-m
+	adding a show_hint method which displays hints if they are enabled	anahita-m
+	adding a show_hint method which displays hints if they are enabled	anahita-m

Diff:

Revision 14 (+181 -57)

Show changes

	.reviewboardrc
	docs/rbtools/rbt/configuration/users.rst
	rbtools/commands/__init__.py
	rbtools/utils/commands.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Thanks, Anahita! Just a few comments below.

Show all issues

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)

Show all issues

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.

docs/rbtools/rbt/configuration/users.rst (Diff revision 14)
Show all issues
```
Typo: -no-hints -> --no-hints.
```

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

Show all issues

Typo: "will speed up will speed up" -> "will speed 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-hints` is added so
		users 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-hints`
		option is working and is also presented as an option to the user when they
		type `rbt [command] --help`.

Commits:

		Author
-	Adding helpful hints for rbt commands	anahita-m
-	adding a show_hint method which displays hints if they are enabled	anahita-m
-	adding a show_hint method which displays hints if they are enabled	anahita-m
+	reverting .reviewboardrc change	anahita-m

Diff:

Revision 15 (+126 -10)

Show changes

	docs/rbtools/rbt/configuration/users.rst
	rbtools/commands/__init__.py
	rbtools/utils/commands.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

You have a pending review.

Adding helpful hints for rbt commands

flake8

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (2 succeeded)

Testing Done:

Testing Done:

Groups:

Testing Done:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (2 succeeded)

Summary:

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (2 succeeded)

Description:

Testing Done:

Commits:

Diff:

Checks run (2 succeeded)

Status: Discarded