Summary

Add support for Gerrit as a hosting service

Review Request #8938 — Created May 11, 2017 and submitted March 7, 2018, 4:39 a.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

~~8923~~, ~~8913~~

Commit

f04d061...

Reviewers

Groups

reviewboard

People

Description

This patch adds support for Gerrit as a hosting service when the Gerrit
server has the gerrit-reviewboard plugin installed (see: /r/8919/ et
al.). The Gerrit API does not provide sufficient information (e.g., it
cannot retrieve files by blob ID, cannot list commits, and cannot
retrieve diffs for arbitrary commits not related to changes), so we rely
on the plugin for this. This hosting service will check for the plugin's
presence via the Gerrit API and will not allow repositories without it
installed.

Testing Done


Tested post-commit review request creation.
Tested rbt post with a Gerrit-backed repository.
Ran unit tests.

Files

Issues

Description	From	Last Updated
F401 'reviewboard.scmtools.errors.SCMError' imported but unused	reviewbot	May 11, 2017, 6:07 p.m.
F401 'hashlib' imported but unused	reviewbot	May 11, 2017, 6:07 p.m.
F401 'hmac' imported but unused	reviewbot	May 11, 2017, 6:07 p.m.
F401 'json' imported but unused	reviewbot	May 11, 2017, 6:07 p.m.
F401 'hashlib.md5' imported but unused	reviewbot	May 11, 2017, 6:07 p.m.
F401 'textwrap.dedent' imported but unused	reviewbot	May 11, 2017, 6:07 p.m.
F401 'django.core.exceptions.ObjectDoesNotExist' imported but unused	reviewbot	May 11, 2017, 6:07 p.m.
F401 'django.utils.six' imported but unused	reviewbot	May 11, 2017, 6:07 p.m.
F401 'django.utils.six.moves.cStringIO as StringIO' imported but unused	reviewbot	May 11, 2017, 6:07 p.m.
F401 'django.utils.six.moves.urllib.error.HTTPError' imported but unused	reviewbot	May 11, 2017, 6:07 p.m.
F401 'django.utils.six.moves.urllib.parse.urlparse' imported but unused	reviewbot	May 11, 2017, 6:07 p.m.
F401 'djblets.testing.decorators.add_fixtures' imported but unused	reviewbot	May 11, 2017, 6:07 p.m.
F401 'reviewboard.hostingsvcs.gerrit.Gerrit' imported but unused	reviewbot	May 11, 2017, 6:07 p.m.
F401 'reviewboard.hostingsvcs.models.HostingServiceAccount' imported but unused	reviewbot	May 11, 2017, 6:07 p.m.
E303 too many blank lines (2)	reviewbot	May 11, 2017, 6:07 p.m.
E128 continuation line under-indented for visual indent	reviewbot	May 11, 2017, 8:07 p.m.
E128 continuation line under-indented for visual indent	reviewbot	May 11, 2017, 8:07 p.m.
E128 continuation line under-indented for visual indent	reviewbot	May 11, 2017, 8:07 p.m.
E501 line too long (84 > 79 characters)	reviewbot	May 11, 2017, 8:07 p.m.
E126 continuation line over-indented for hanging indent	reviewbot	May 11, 2017, 8:07 p.m.
"subclasses"	chipx86	July 21, 2017, 8:49 p.m.
I think you meant "hosting account"?	chipx86	July 21, 2017, 8:49 p.m.
t comes after s.	david	May 23, 2017, 7:55 a.m.
Wrap in parens instead of using the continuation character.	david	May 23, 2017, 8 a.m.
The Meta class should come last in the form.	chipx86	July 21, 2017, 8:51 p.m.
The > can be inside the <tt>. In other words, one <tt> should be fine.	chipx86	July 21, 2017, 8:51 p.m.
Here too.	chipx86	July 21, 2017, 8:51 p.m.
Missing : after :py:meth.	chipx86	July 21, 2017, 8:51 p.m.
"hostingsvcs"	chipx86	July 21, 2017, 9:09 p.m.
"project form?"	chipx86	July 21, 2017, 9:10 p.m.
Seems like this should maybe be "Gerrit URL"?	chipx86	July 21, 2017, 9:11 p.m.
This is missing help text.	chipx86	July 21, 2017, 9:11 p.m.
This should be "Project name". It's also missing help text.	chipx86	July 21, 2017, 9:12 p.m.
Wrap this error in _()?	david	May 23, 2017, 8:01 a.m.
The error should be less terse. Maybe "This is not a valid port. Ports must be in the range of …	chipx86	July 21, 2017, 9:13 p.m.
Might read better as "The only required part is the FQDN ...	chipx86	July 21, 2017, 9:14 p.m.
typo: ot	david	May 23, 2017, 8:01 a.m.
Looks like this could be one line?	chipx86	July 21, 2017, 9:14 p.m.
This can be one statement.	chipx86	July 21, 2017, 9:15 p.m.
What happens if this changes in the future? Think we should code defensively here. If the data doesn't include this, …	chipx86	July 21, 2017, 9:32 p.m.
We only use this in one error message that's only called when configuring a repository, so let's do that there …	chipx86	July 21, 2017, 9:33 p.m.
Probably belongs up with form.	chipx86	July 21, 2017, 9:33 p.m.
This is the default.	chipx86	July 21, 2017, 9:34 p.m.
You don't have to provide this argument. It will be captured by kwargs.	chipx86	July 21, 2017, 9:34 p.m.
No need to provide this either.	chipx86	July 21, 2017, 9:34 p.m.
Missing , optional.	chipx86	July 21, 2017, 9:34 p.m.
Missing , optional.	chipx86	July 21, 2017, 10:27 p.m.
Needs the full class path.	chipx86	July 21, 2017, 9:36 p.m.
e should be passed in as another arg, since the logger functions do their own format operation.	david	May 23, 2017, 8:01 a.m.
Ideally we should flesh this out. "Unable to authenticate with Gerrit. The username or password might be invalid." It's best …	chipx86	July 21, 2017, 9:38 p.m.
Missing docs.	chipx86	July 21, 2017, 9:39 p.m.
We should be using encrypt_password and decrypt_password for any credentials being stored.	chipx86	July 21, 2017, 9:50 p.m.
Typo: "reviewbaord" -> "reviewboard"	chipx86	July 21, 2017, 9:50 p.m.
This can only be done if we're shipping this with 3.0. We still support Python 2.6 for Review Board 2.5.	chipx86	July 21, 2017, 10:16 p.m.
I'd suggest following GitHub's model and defining an api_get that does the common stuff (passing credentials and only returning the …	chipx86	July 21, 2017, 10:11 p.m.
I am left in suspense.	chipx86	July 21, 2017, 10:12 p.m.
Historically, this function comes before the rest of the overriden functions (this, then authorize, then the rest).	chipx86	July 21, 2017, 10:12 p.m.
Typo: "reviewbaord" -> "reviewboard"	chipx86	July 21, 2017, 10:12 p.m.
You can do: parts = [ ... ] + rest_parts	chipx86	July 21, 2017, 10:13 p.m.
Better formatted as: url = ('%s/' % urljoin(...)) I believe that will fit on two lines. Maybe three, if you …	chipx86	July 21, 2017, 10:13 p.m.
Should be one statement.	chipx86	July 21, 2017, 10:14 p.m.
Can you add two blank lines between these?	david	May 23, 2017, 8:02 a.m.
Undo this change.	david	May 23, 2017, 8:02 a.m.
Undo this change.	david	May 23, 2017, 8:02 a.m.
Undo this change.	david	May 23, 2017, 8:02 a.m.
Undo this change. The .closest is part of the whole selector process. The .setVisible is the thing we're calling on …	chipx86	July 21, 2017, 10:15 p.m.
Can you add two blank lines between these?	david	May 23, 2017, 8:03 a.m.
E999 SyntaxError: invalid syntax	reviewbot	July 21, 2017, 10:28 p.m.
E501 line too long (80 > 79 characters)	reviewbot	July 21, 2017, 10:28 p.m.
E303 too many blank lines (2)	reviewbot	July 21, 2017, 10:28 p.m.
E303 too many blank lines (2)	reviewbot	July 21, 2017, 10:29 p.m.
W391 blank line at end of file	reviewbot	July 21, 2017, 10:29 p.m.
This should go after all the django.utils.six imports	david	Jan. 10, 2018, 10:01 p.m.
Use parens instead of \	david	Jan. 10, 2018, 10:01 p.m.
Trailing comma isn't necessary since these are just args. ) can be on this line too.	david	Jan. 10, 2018, 10:02 p.m.
Should be marked for translation.	david	Jan. 10, 2018, 10:02 p.m.
Should be marked for translation.	david	Jan. 10, 2018, 10:02 p.m.
Should end in a period to match the others.	david	Jan. 10, 2018, 10:02 p.m.
Instead of implementing a custom clean method you should be able to just pass in min_value and max_value to the …	david	Jan. 10, 2018, 10:03 p.m.
Other implementations call this arg hosting_service	david	Jan. 10, 2018, 10:03 p.m.
typo: ot	david	Jan. 10, 2018, 10:03 p.m.
Comma after "Otherwise"	david	Jan. 10, 2018, 10:04 p.m.
The string included in the comment here doesn't match the code.	david	Jan. 10, 2018, 10:06 p.m.
The implementation of _parse_plugin_version could raise exceptions if we get something unexpected from the server. Can we try/catch and convert …	david	Jan. 10, 2018, 10:15 p.m.
Blank line after this.	david	Jan. 10, 2018, 10:15 p.m.
Add two blank lines after this.	david	Jan. 10, 2018, 10:18 p.m.
Undo this change. Maybe wrap the whole thing in parens too?	david	Jan. 10, 2018, 10:19 p.m.
Undo this change.	david	Jan. 10, 2018, 10:19 p.m.
Undo this change.	david	Jan. 10, 2018, 10:19 p.m.
Undo this change.	david	Jan. 10, 2018, 10:20 p.m.
Undo this change.	david	Jan. 10, 2018, 10:20 p.m.
Add two blank lines before this.	david	Jan. 10, 2018, 10:20 p.m.
F401 'django.core.exceptions.ValidationError' imported but unused	reviewbot	Jan. 12, 2018, 11:54 p.m.
E203 whitespace before ':'	reviewbot	Jan. 12, 2018, 11:54 p.m.
E231 missing whitespace after ':'	reviewbot	Jan. 12, 2018, 11:54 p.m.
E701 multiple statements on one line (colon)	reviewbot	Jan. 12, 2018, 11:54 p.m.
E999 SyntaxError: invalid syntax	reviewbot	Jan. 12, 2018, 11:54 p.m.
We should probably assert that gerrit_url is not None here.	david	Feb. 1, 2018, 11:31 p.m.
Leftover debug output?	david	Feb. 1, 2018, 11:32 p.m.
Leftover debug output?	david	Feb. 1, 2018, 11:32 p.m.
I think this would be more readable if we split it up a bit: url = urljoin(repository.extra_data['gerrit_url'], '/'.join(parts)) if query: …	david	Feb. 1, 2018, 11:33 p.m.
Why this change?	david	Feb. 1, 2018, 11:34 p.m.
Blank line after this.	david	Feb. 8, 2018, 10:03 p.m.
So in python 3, json.dumps requires that all the keys and values be str. I think instead of having a …	david	Feb. 8, 2018, 10:04 p.m.
Same here.	david	Feb. 8, 2018, 10:04 p.m.
And here.	david	Feb. 8, 2018, 10:04 p.m.
And here.	david	Feb. 8, 2018, 10:05 p.m.
And here.	david	Feb. 8, 2018, 10:05 p.m.
Blank line after this.	david	Feb. 8, 2018, 10:06 p.m.
And here.	david	Feb. 8, 2018, 10:06 p.m.
E131 continuation line unaligned for hanging indent	reviewbot	Feb. 9, 2018, 5:59 a.m.
Because we know the format, this would be a lot more readable as: 'required': '%d.%d.%d' % self.REQUIRED_PLUGIN_VERSION,	david	Feb. 10, 2018, 9:27 p.m.
Please pass in e as an arg to logging.error instead of pre-formatting it.	david	Feb. 10, 2018, 9:28 p.m.
This should fit on one line.	david	Feb. 10, 2018, 9:28 p.m.
This can just use + instead of extend	david	Feb. 12, 2018, 8:11 p.m.
F401 'django.utils.six' imported but unused	reviewbot	Feb. 10, 2018, 9:31 p.m.
Swap these.	chipx86	Feb. 16, 2018, 4:30 a.m.
Should be in alphabetical order.	chipx86	Feb. 16, 2018, 4:32 a.m.
I know the base implementation doesn't do anything, but it might in the future. Let's call the parent method here.	chipx86	Feb. 16, 2018, 4:32 a.m.
"to the"	chipx86	Feb. 16, 2018, 4:32 a.m.
We should specify the URL here (first argument in *args), since we otherwise don't have enough information to figure out …	chipx86	Feb. 16, 2018, 4:34 a.m.
Can we have a module-level logger?	chipx86	Feb. 16, 2018, 5:15 a.m.
"Source code"	chipx86	Feb. 16, 2018, 4:37 a.m.
Mind linking to the URL here?	chipx86	Feb. 16, 2018, 5:15 a.m.
No need for the backtick when using a reference without a space in the name.	chipx86	March 1, 2018, 9:33 p.m.
"Gerrit"	chipx86	Feb. 16, 2018, 4:37 a.m.
The ] shouldn't be here.	chipx86	Feb. 16, 2018, 4:37 a.m.
We should link to the URL for the repository in this message. We can then provide instructions in that repository's …	chipx86	Feb. 16, 2018, 4:40 a.m.
This can fit on one line.	chipx86	March 1, 2018, 9:33 p.m.
We should specify the repository ID here, since we otherwise don't have enough information to debug problems.	chipx86	Feb. 22, 2018, 12:40 a.m.
Keyword arguments should ideally be used in localized text when there's more than one variable being injected. Helps with translations …	chipx86	Feb. 16, 2018, 4:42 a.m.
The dictionary should be expanded.	chipx86	March 1, 2018, 9:33 p.m.
Same here.	chipx86	March 1, 2018, 9:34 p.m.
We should specify the URL here, since we otherwise don't have enough information to debug problems.	chipx86	Feb. 16, 2018, 4:42 a.m.
Missing gettext.	chipx86	Feb. 16, 2018, 4:44 a.m.
Missing gettext.	chipx86	Feb. 16, 2018, 4:45 a.m.
The dictionary should be expanded.	chipx86	March 1, 2018, 9:34 p.m.
Same here.	chipx86	March 1, 2018, 9:34 p.m.
Parens around the equality check.	chipx86	Feb. 16, 2018, 4:45 a.m.
Missing gettext.	chipx86	Feb. 16, 2018, 4:47 a.m.
Missing gettext.	chipx86	Feb. 16, 2018, 4:47 a.m.
Same here.	chipx86	March 1, 2018, 9:39 p.m.
Same here.	chipx86	March 1, 2018, 9:39 p.m.
gettext	chipx86	Feb. 16, 2018, 4:48 a.m.
Can we catch an exception here and handle it gracefully, in case something goes wonky with the response?	chipx86	Feb. 16, 2018, 4:50 a.m.
Same here.	chipx86	March 1, 2018, 9:41 p.m.
We should specify the repository ID here, since we otherwise don't have enough information to debug problems.	chipx86	Feb. 16, 2018, 4:51 a.m.
We should ideally use keywords for the format string here.	chipx86	Feb. 16, 2018, 4:51 a.m.
Same here.	chipx86	March 1, 2018, 9:41 p.m.
Can you place arguments starting right after Commit(? These aren't long enough to require wrapping.	chipx86	Feb. 16, 2018, 4:51 a.m.
Same here.	chipx86	March 1, 2018, 9:41 p.m.
Should use +=. It's faster. Or just + on the previous line.	chipx86	Feb. 16, 2018, 4:52 a.m.
Alphabetical order.	chipx86	March 1, 2018, 9:42 p.m.
Alphabetical order.	chipx86	Feb. 16, 2018, 4:52 a.m.
Private methods go last.	chipx86	Feb. 16, 2018, 4:53 a.m.
You can start after Repository(	chipx86	Feb. 16, 2018, 4:53 a.m.
This shouldn't be necessary on Gerrit. Same with other calls in this file.	chipx86	Feb. 16, 2018, 4:54 a.m.
The parens aren't necessary in JavaScript.	chipx86	Feb. 16, 2018, 4:55 a.m.
E501 line too long (82 > 79 characters)	reviewbot	Feb. 16, 2018, 5:17 a.m.
E501 line too long (92 > 79 characters)	reviewbot	Feb. 16, 2018, 5:17 a.m.
E501 line too long (81 > 79 characters)	reviewbot	Feb. 16, 2018, 5:17 a.m.
E127 continuation line over-indented for visual indent	reviewbot	Feb. 16, 2018, 5:17 a.m.
W391 blank line at end of file	reviewbot	Feb. 16, 2018, 5:17 a.m.
E501 line too long (92 > 79 characters)	reviewbot	Feb. 21, 2018, 10:59 p.m.
E501 line too long (92 > 79 characters)	reviewbot	March 1, 2018, 9:32 p.m.
E501 line too long (92 > 79 characters)	reviewbot	March 1, 2018, 9:48 p.m.
I just realized this isn't going to work. e.read() makes no sense for a b64decode. The error isn't really descriptive …	chipx86	March 7, 2018, 2:53 a.m.
This needs to be unfolded as well.	chipx86	March 7, 2018, 2:51 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/hostingsvcs/gerrit.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'reviewboard.scmtools.errors.SCMError' imported but unused
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'hashlib' imported but unused
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'hmac' imported but unused
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'json' imported but unused
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'hashlib.md5' imported but unused
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'textwrap.dedent' imported but unused
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'django.core.exceptions.ObjectDoesNotExist' imported but unused
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'django.utils.six' imported but unused
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'django.utils.six.moves.cStringIO as StringIO' imported but unused
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'django.utils.six.moves.urllib.error.HTTPError' imported but unused
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'django.utils.six.moves.urllib.parse.urlparse' imported but unused
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'djblets.testing.decorators.add_fixtures' imported but unused
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'reviewboard.hostingsvcs.gerrit.Gerrit' imported but unused
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 1)
The issue has been dropped. Show all issues
```
F401 'reviewboard.hostingsvcs.models.HostingServiceAccount' imported but unused
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 1)
The issue has been dropped. Show all issues
```
E303 too many blank lines (2)
```

Change Summary:

Unit tests.

Summary:

WIP: Gerrit Support

Add support for Gerrit as a hosting service

Description:

~		WIP
	~	This patch adds support for Gerrit as a hosting service when the Gerrit
	+	server has the `gerrit-reviewboard` plugin installed (see: /r/8919/ et
	+	al.). The Gerrit API does not provide sufficient information (e.g., it
	+	cannot retrieve files by blob ID, cannot list commits, and cannot
	+	retrieve diffs for arbitrary commits not related to changes), so we rely
	+	on the plugin for this. This hosting service will check for the plugin's
	+	presence via the Gerrit API and will not allow repositories without it
	+	installed.

Testing Done:

	+	Tested post-commit review request creation.
	+	Tested `rbt post` with a Gerrit-backed repository.
	+	Ran unit tests.

Diff:

Revision 2 (+1044 -13)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E501 line too long (84 > 79 characters)
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 2)
The issue has been resolved. Show all issues
```
E126 continuation line over-indented for hanging indent
```

Added Files:

Change Summary:

flake8

Diff:

Revision 3 (+1045 -13)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)

The issue has been resolved. Show all issues

t comes after s.

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)

The issue has been resolved. Show all issues

Wrap in parens instead of using the continuation character.

brennie

May 23, 2017, 8 a.m.

I discovered pycharm auto imports while doing the java stuff but it doesnt do it so pretty.

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Wrap this error in _()?
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
typo: ot
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)

The issue has been resolved. Show all issues

e should be passed in as another arg, since the logger functions do their own format operation.

reviewboard/static/rb/js/admin/repositoryform.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Can you add two blank lines between these?
```
reviewboard/static/rb/js/admin/repositoryform.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Undo this change.
```
reviewboard/static/rb/js/admin/repositoryform.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Undo this change.
```
reviewboard/static/rb/js/admin/repositoryform.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Undo this change.
```
reviewboard/static/rb/js/admin/repositoryform.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Can you add two blank lines between these?
```

reviewboard/hostingsvcs/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"subclasses"
```
reviewboard/hostingsvcs/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
I think you meant "hosting account"?
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
The Meta class should come last in the form.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
The &gt; can be inside the <tt>. In other words, one <tt> should be fine.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Here too.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing : after :py:meth.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"hostingsvcs"
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"project form?"
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Seems like this should maybe be "Gerrit URL"?
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This is missing help text.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This should be "Project name".
It's also missing help text.
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)

The issue has been resolved. Show all issues

The error should be less terse. Maybe "This is not a valid port. Ports must be in the range of 0-65535."

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Might read better as "The only required part is the FQDN ...
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Looks like this could be one line?
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This can be one statement.
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)

The issue has been resolved. Show all issues

What happens if this changes in the future? Think we should code defensively here. If the data doesn't include this, log it and return the data as-is.  Might also want to make the \n optional, as that seems like an easy thing that can change.

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)

The issue has been resolved. Show all issues

We only use this in one error message that's only called when configuring a repository, so let's do that there instead.

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Probably belongs up with form.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This is the default.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
You don't have to provide this argument. It will be captured by kwargs.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
No need to provide this either.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing , optional.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Missing , optional.
```
1. brennie
  
  July 21, 2017, 10:27 p.m.
  Not optional.
reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Needs the full class path.
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)

The issue has been resolved. Show all issues

Ideally we should flesh this out. "Unable to authenticate with Gerrit. The username or password might be invalid."

It's best to avoid overly terse errors that look like they came from Posix libraries.

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing docs.
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)

The issue has been resolved. Show all issues

We should be using encrypt_password and decrypt_password for any credentials being stored.

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Typo: "reviewbaord" -> "reviewboard"
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)

The issue has been resolved. Show all issues

I'd suggest following GitHub's model and defining an api_get that does the common stuff (passing credentials and only returning the data).

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
I am left in suspense.
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)

The issue has been resolved. Show all issues

Historically, this function comes before the rest of the overriden functions (this, then authorize, then the rest).

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Typo: "reviewbaord" -> "reviewboard"
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)

The issue has been resolved. Show all issues

You can do:

parts = [
    ...
] + rest_parts

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)

The issue has been resolved. Show all issues

Better formatted as:

url = ('%s/'
       % urljoin(...))


I believe that will fit on two lines. Maybe three, if you need to put '/'.join(parts) on the next line.

reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Should be one statement.
```

reviewboard/static/rb/js/admin/repositoryform.js (Diff revision 3)

The issue has been dropped. Show all issues

Undo this change. The .closest is part of the whole selector process. The .setVisible is the thing we're calling on the resulting elements.

brennie

July 21, 2017, 10:15 p.m.

This is in contravention to basically the rest of our JS style.

david

July 21, 2017, 10:19 p.m.

I think this would be the best:

$twoFactorAuthRows = $authForm
    .find('[data-required-for-2fa]')
    .closest('.form-row')
        .setVisible(hostingInfo.needs_two_factor_auth_code);

reviewboard/hostingsvcs/gerrit.py (Diff revision 3)

The issue has been resolved. Show all issues

This can only be done if we're shipping this with 3.0. We still support Python 2.6 for Review Board 2.5.

Change Summary:

Addressed Christian's feedback. Need to doublecheck everything still works.

Diff:

Revision 4 (+1183 -15)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/hostingsvcs/gerrit.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E501 line too long (80 > 79 characters)
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 4)
The issue has been resolved. Show all issues
```
W391 blank line at end of file
```

Commit:

132ec30a34ede4c145c351ae1bb63c42eeafd408

Diff:

Revision 5 (+1185 -15)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This should go after all the django.utils.six imports
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Use parens instead of \
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 5)

The issue has been resolved. Show all issues

Trailing comma isn't necessary since these are just args. ) can be on this line too.

reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Should be marked for translation.
```
1. brennie
  
  Jan. 10, 2018, 10:20 p.m.
  It is.
reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Should be marked for translation.
```
1. brennie
  
  Jan. 10, 2018, 10:20 p.m.
  It is.
reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Should end in a period to match the others.
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 5)

The issue has been resolved. Show all issues

Instead of implementing a custom clean method you should be able to just pass in min_value and max_value to the forms.IntegerField constructor.

reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Other implementations call this arg hosting_service
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
The issue has been resolved. Show all issues
```
typo: ot
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Comma after "Otherwise"
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
The issue has been resolved. Show all issues
```
The string included in the comment here doesn't match the code.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
```
Does this need a try/catch?
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 5)

The issue has been resolved. Show all issues

The implementation of _parse_plugin_version could raise exceptions if we get something unexpected from the server. Can we try/catch and convert to a RepositoryError?

reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
```
Does this need a try/catch?
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Blank line after this.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 5)
```
Does this need a try/catch?
```
reviewboard/static/rb/js/admin/repositoryform.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Add two blank lines after this.
```
reviewboard/static/rb/js/admin/repositoryform.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Undo this change. Maybe wrap the whole thing in parens too?
```
reviewboard/static/rb/js/admin/repositoryform.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Undo this change.
```
reviewboard/static/rb/js/admin/repositoryform.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Undo this change.
```
reviewboard/static/rb/js/admin/repositoryform.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Undo this change.
```
reviewboard/static/rb/js/admin/repositoryform.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Undo this change.
```
reviewboard/static/rb/js/admin/repositoryform.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Add two blank lines before this.
```

Change Summary:

Addressed David's feedback

Commit:

132ec30a34ede4c145c351ae1bb63c42eeafd408

94bd97fdcb38df77c798472722c37a0e661f712a

Diff:

Revision 6 (+1194 -13)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/hostingsvcs/gerrit.py (Diff revision 6)
The issue has been resolved. Show all issues
```
F401 'django.core.exceptions.ValidationError' imported but unused
```

reviewboard/hostingsvcs/github.py (Diff revision 6)

The issue has been resolved. Show all issues

E203 whitespace before ':'

chipx86

Jan. 11, 2018, 6:09 a.m.

Make sure you've tested these changes before posting for review. These syntax errors should have stood out immediately.

reviewboard/hostingsvcs/github.py (Diff revision 6)
The issue has been resolved. Show all issues
```
E231 missing whitespace after ':'
```
reviewboard/hostingsvcs/github.py (Diff revision 6)
The issue has been resolved. Show all issues
```
E701 multiple statements on one line (colon)
```
reviewboard/hostingsvcs/github.py (Diff revision 6)
The issue has been resolved. Show all issues
```
E999 SyntaxError: invalid syntax
```

Change Summary:

Fixed typo and test failures.
Fix branch retrieval.

Manual testing.

Commit:

94bd97fdcb38df77c798472722c37a0e661f712a

195458a9e609e56847ec82e6d7f24550d866bea2

Diff:

Revision 7 (+1194 -12)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Publish right changeset

Commit:

195458a9e609e56847ec82e6d7f24550d866bea2

9b33de326d3e740febd6ec18cfb48e29a336a30d

Diff:

Revision 8 (+1213 -12)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/hostingsvcs/gerrit.py (Diff revision 8)
The issue has been resolved. Show all issues
```
We should probably assert that gerrit_url is not None here.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Leftover debug output?
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Leftover debug output?
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 8)

The issue has been resolved. Show all issues

I think this would be more readable if we split it up a bit:

url = urljoin(repository.extra_data['gerrit_url'],
              '/'.join(parts))

if query:
    url = '%s/?%s' % (url, urlencode(query))
else:
    url = '%s/' % url

reviewboard/hostingsvcs/github.py (Diff revision 8)
The issue has been resolved. Show all issues
```
Why this change?
```

Change Summary:

Addressed David's feedback

Commit:

9b33de326d3e740febd6ec18cfb48e29a336a30d

639c85ab2d5a1df87bfe49e9aa0814c90c1618d3

Diff:

Revision 9 (+1214 -11)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/hostingsvcs/gerrit.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Blank line after this.
```

reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 9)

The issue has been resolved. Show all issues

So in python 3, json.dumps requires that all the keys and values be str. I think instead of having a bunch of bytestrings in here, we should have them all be unicode, but then do json.dumps(...).encode('utf-8')

reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 9)
The issue has been resolved. Show all issues
```
And here.
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 9)
The issue has been resolved. Show all issues
```
And here.
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 9)
The issue has been resolved. Show all issues
```
And here.
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Blank line after this.
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 9)
The issue has been resolved. Show all issues
```
And here.
```

Change Summary:

Addressed David's feedback.

Commit:

639c85ab2d5a1df87bfe49e9aa0814c90c1618d3

2049fb6d13a537310bd7fba1dc37c32d24bfba50

Diff:

Revision 10 (+1215 -11)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 10)
The issue has been resolved. Show all issues
```
E131 continuation line unaligned for hanging indent
```

Commit:

2049fb6d13a537310bd7fba1dc37c32d24bfba50

281ec69a84c890149fe2c5f6562f5a9ce7cbc3d5

Diff:

Revision 11 (+1215 -11)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/hostingsvcs/gerrit.py (Diff revision 11)

The issue has been resolved. Show all issues

Because we know the format, this would be a lot more readable as:

'required': '%d.%d.%d' % self.REQUIRED_PLUGIN_VERSION,

reviewboard/hostingsvcs/gerrit.py (Diff revision 11)
The issue has been resolved. Show all issues
```
Please pass in e as an arg to logging.error instead of pre-formatting it.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 11)
The issue has been resolved. Show all issues
```
This should fit on one line.
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 11)

The issue has been dropped. Show all issues

This can just use + instead of extend

brennie

Feb. 10, 2018, 9:29 p.m.
```
Not if its a tuple.
```

david

Feb. 12, 2018, 8:11 p.m.


>>> (1, 2, 3) + (4, 5)
(1, 2, 3, 4, 5)

david

Feb. 12, 2018, 8:11 p.m.

Oh, I see, you're appending a tuple to a list. Nevermind then.

Change Summary:

Addressed David's feedback.

Commit:

281ec69a84c890149fe2c5f6562f5a9ce7cbc3d5

d7ca381374c4239518f524add0f7bfa44f9d2a80

Diff:

Revision 12 (+1211 -11)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/hostingsvcs/gerrit.py (Diff revision 12)
The issue has been resolved. Show all issues
```
F401 'django.utils.six' imported but unused
```

Change Summary:

Linting.

Commit:

d7ca381374c4239518f524add0f7bfa44f9d2a80

1f0aa4dfbc6c40684fe348a89596a65d9eeb6ab2

Diff:

Revision 13 (+1210 -11)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

Looks good. Please re-run manual and automated tests before pushing.

This looks like more feedback than it is. Primarily some small docs/error message stuff repeated for different instances.

reviewboard/hostingsvcs/gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Swap these.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Should be in alphabetical order.
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 13)

The issue has been resolved. Show all issues

I know the base implementation doesn't do anything, but it might in the future. Let's call the parent method here.

reviewboard/hostingsvcs/gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
"to the"
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 13)

The issue has been resolved. Show all issues

We should specify the URL here (first argument in *args), since we otherwise don't have enough information to figure out what went wrong.

reviewboard/hostingsvcs/gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Can we have a module-level logger?
```
1. brennie
  
  Feb. 16, 2018, 4:57 a.m.
  I thought we couldn't use module-level loggers for some reason?
reviewboard/hostingsvcs/gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
"Source code"
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Mind linking to the URL here?
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
"Gerrit"
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
The ] shouldn't be here.
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 13)

The issue has been resolved. Show all issues

We should link to the URL for the repository in this message. We can then provide instructions in that repository's README for installation instructions.

reviewboard/hostingsvcs/gerrit.py (Diff revision 13)

The issue has been resolved. Show all issues

We should specify the repository ID here, since we otherwise don't have enough information to debug problems.

brennie

Feb. 16, 2018, 4:57 a.m.

We don't have a repository ID here, unless I am mistaken.

chipx86

Feb. 22, 2018, 12:36 a.m.

You're right, we don't. We can include the Gerrit URL in this case.

reviewboard/hostingsvcs/gerrit.py (Diff revision 13)

The issue has been resolved. Show all issues

Keyword arguments should ideally be used in localized text when there's more than one variable being injected. Helps with translations when the order ends up changing.

reviewboard/hostingsvcs/gerrit.py (Diff revision 13)

The issue has been resolved. Show all issues

We should specify the URL here, since we otherwise don't have enough information to debug problems.

reviewboard/hostingsvcs/gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Missing gettext.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Missing gettext.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Parens around the equality check.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Missing gettext.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Missing gettext.
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
gettext
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 13)

The issue has been resolved. Show all issues

Can we catch an exception here and handle it gracefully, in case something goes wonky with the response?

reviewboard/hostingsvcs/gerrit.py (Diff revision 13)

The issue has been resolved. Show all issues

We should specify the repository ID here, since we otherwise don't have enough information to debug problems.

reviewboard/hostingsvcs/gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
We should ideally use keywords for the format string here.
```

reviewboard/hostingsvcs/gerrit.py (Diff revision 13)

The issue has been resolved. Show all issues

Can you place arguments starting right after Commit(? These aren't long enough to require wrapping.

reviewboard/hostingsvcs/gerrit.py (Diff revision 13)

The issue has been dropped. Show all issues

Should use +=. It's faster. Or just + on the previous line.

brennie

Feb. 16, 2018, 4:57 a.m.

Can't if rest_parts is a tuple, like I mentioned in a previous review.

reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
Alphabetical order.
```

reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 13)

The issue has been resolved. Show all issues

Private methods go last.

reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
You can start after Repository(
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 13)
The issue has been resolved. Show all issues
```
This shouldn't be necessary on Gerrit.
Same with other calls in this file.
```
reviewboard/static/rb/js/admin/repositoryform.js (Diff revision 13)
The issue has been resolved. Show all issues
```
The parens aren't necessary in JavaScript.
```

Commit:

1f0aa4dfbc6c40684fe348a89596a65d9eeb6ab2

31bdb1f97a0c065362f617be8515ea3982d75e03

Diff:

Revision 14 (+1231 -9)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/hostingsvcs/gerrit.py (Diff revision 14)
The issue has been resolved. Show all issues
```
E501 line too long (82 > 79 characters)
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 14)
The issue has been dropped. Show all issues
```
E501 line too long (92 > 79 characters)
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 14)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```
reviewboard/hostingsvcs/gerrit.py (Diff revision 14)
The issue has been resolved. Show all issues
```
E127 continuation line over-indented for visual indent
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revision 14)
The issue has been resolved. Show all issues
```
W391 blank line at end of file
```

Commit:

31bdb1f97a0c065362f617be8515ea3982d75e03

6fad3546f04264c9f312cb0fc60a1f831b578ea0

Diff:

Revision 15 (+1231 -9)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/hostingsvcs/gerrit.py (Diff revision 15)
The issue has been dropped. Show all issues
```
E501 line too long (92 > 79 characters)
```

Commit:

6fad3546f04264c9f312cb0fc60a1f831b578ea0

ad10029713c4314896eb8ca1f46b3a1170e0fbca

Diff:

Revision 16 (+1237 -9)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/hostingsvcs/gerrit.py (Diff revision 16)

The issue has been resolved. Show all issues

E501 line too long (92 > 79 characters)

chipx86

Feb. 24, 2018, 7:11 a.m.

You can put the URL on the next line, like:

.. _gerrit-reviewboard-plugin:
   https://....

reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
The issue has been resolved. Show all issues
```
No need for the backtick when using a reference without a space in the name.
```
reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
The issue has been resolved. Show all issues
```
This can fit on one line.
```
reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
The issue has been resolved. Show all issues
```
The dictionary should be expanded.
```
reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
The issue has been resolved. Show all issues
```
The dictionary should be expanded.
```
reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/hostingsvcs/gerrit.py (Diff revisions 13 - 16)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/hostingsvcs/tests/test_gerrit.py (Diff revisions 13 - 16)
The issue has been resolved. Show all issues
```
Alphabetical order.
```

Commit:

ad10029713c4314896eb8ca1f46b3a1170e0fbca

4404c4ee7df5a5ca36c3a62058727dcef72fbaf7

Diff:

Revision 17 (+1237 -9)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/hostingsvcs/gerrit.py (Diff revision 17)
The issue has been dropped. Show all issues
```
E501 line too long (92 > 79 characters)
```

Change Summary:

post correct revision.

Commit:

4404c4ee7df5a5ca36c3a62058727dcef72fbaf7

602caaea3acf1909666239d5447cb1435be4dd58

Diff:

Revision 18 (+1259 -9)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/hostingsvcs/gerrit.py (Diff revision 18)

The issue has been resolved. Show all issues

I just realized this isn't going to work. e.read() makes no sense for a b64decode. The error isn't really descriptive of the problem, either. Can you fix this up and add a unit test for it?

reviewboard/hostingsvcs/gerrit.py (Diff revision 18)
The issue has been resolved. Show all issues
```
This needs to be unfolded as well.
```

Commit:

602caaea3acf1909666239d5447cb1435be4dd58

86ed503416778e770e67577804cc1b692e2256d1

Diff:

Revision 19 (+1264 -9)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

86ed503416778e770e67577804cc1b692e2256d1

f04d061dbef9a891d10e7a9b7604e006deebedf1

Diff:

Revision 20 (+1280 -9)

Show changes

	setup.py
	reviewboard/hostingsvcs/forms.py
	reviewboard/hostingsvcs/gerrit.py
	reviewboard/hostingsvcs/tests/test_gerrit.py
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/js/admin/repositoryform.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.5.x (308de19)