Summary

EasyFix: Make flake8 an optional dependency under "all"

Review Request #10151 — Created Sept. 21, 2018 and submitted Oct. 17, 2018, 11:56 a.m.

Information

Owner

alextechcc

Repository

ReviewBot

Branch

master

Bugs

Depends On

Blocks

10218

Commit

4815c03...

Reviewers

Groups

reviewbot, students

People

Description

The new "all" optional dependency target is intended to replicate the
old behavior of installing all python dependencies.

Documentation has been updated to reflect this.

Testing Done

Since this is just a change for pip dependencies I've done:

- pip install -e ./bot reviewbot-worker

- pip install -e ./bot reviewbot-worker[all]

To test if it installs and doesn't install flake8 under the default 

package and the all target, and according to the logs this is the case.

Issues

Description	From	Last Updated
Please wrap your testing done to 72 columns (it goes into the commit message)	david	Sept. 21, 2018, 10:15 a.m.
Can you fix the summary to say "all" instead of "ALL"?	david	Sept. 22, 2018, 6:42 a.m.
Can we call this "all" instead of "ALL"? Also, a couple formatting nits: One space between the : and the …	david	Sept. 21, 2018, 10:15 a.m.
Can we say "pip install flake8" so people can just copy and paste?	david	Sept. 21, 2018, 10:15 a.m.

flake8 passed.

JSHint passed.

Change Summary:

Updated documentation to reflect flake8 not being handled by installing reviewbot-worker

Description:

		The new "ALL" optional dependency target is intended to replicate the
		old behavior of installing all python dependencies.
	+
	+	Documentation has been updated to reflect this.

Commit:

-	0c731112a7613fb94fb9983242de5a22bd1d850c
+	e3d973014a7a78fd98b029196e8a6204989a74e4

Diff:

Revision 2 (+5 -3)

Show changes

	bot/setup.py
	docs/reviewbot/tools/flake8.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Please wrap your testing done to 72 columns (it goes into the commit message)

bot/setup.py (Diff revision 1)

The issue has been resolved. Show all issues

Can we call this "all" instead of "ALL"?

Also, a couple formatting nits:

One space between the : and the [
Let's split the list into one line per item, since we know we'll be adding more to it in the future.

docs/reviewbot/tools/flake8.rst (Diff revision 2)
The issue has been resolved. Show all issues
```
Can we say "pip install flake8" so people can just copy and paste?
```

Change Summary:

Fixed style and added full pip command to install flake8

Testing Done:

		Since this is just a change for pip dependencies I've done:
		- `pip install -e ./bot reviewbot-worker`
		- `pip install -e ./bot reviewbot-worker[ALL]`
~		To test if it installs and doesn't install flake8 under the default package and the ALL target, and according to the logs this is the case.
	~	To test if it installs and doesn't install flake8 under the default
	+	package and the ALL target, and according to the logs this is the case.

Commit:

-	e3d973014a7a78fd98b029196e8a6204989a74e4
+	4815c03fce6ce3631a95b36ffbdb496e34ab4c42

Diff:

Revision 3 (+7 -3)

Show changes

	bot/setup.py
	docs/reviewbot/tools/flake8.rst

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

~		The new "ALL" optional dependency target is intended to replicate the
	~	The new "all" optional dependency target is intended to replicate the
		old behavior of installing all python dependencies.

		Documentation has been updated to reflect this.

Testing Done:

		Since this is just a change for pip dependencies I've done:
		- `pip install -e ./bot reviewbot-worker`
~		- `pip install -e ./bot reviewbot-worker[ALL]`
	~	- `pip install -e ./bot reviewbot-worker[all]`
		To test if it installs and doesn't install flake8 under the default
		package and the ALL target, and according to the logs this is the case.

The issue has been resolved. Show all issues

Can you fix the summary to say "all" instead of "ALL"?

Summary:

-	EasyFix: Make flake8 an optional dependency under "ALL"
+	EasyFix: Make flake8 an optional dependency under "all"

Testing Done:

		Since this is just a change for pip dependencies I've done:
		- `pip install -e ./bot reviewbot-worker`
		- `pip install -e ./bot reviewbot-worker[all]`
		To test if it installs and doesn't install flake8 under the default
~		package and the ALL target, and according to the logs this is the case.
	~	package and the all target, and according to the logs this is the case.

Ship it!

```
Looks good to me - thanks!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (1e8a677)