flake8
-
rbintegrations/discord/tests.py (Diff revision 1) Show all issues -
Review Request #11201 — Created Sept. 24, 2020 and submitted
Information | |
---|---|
hailan | |
rbintegrations | |
master | |
Reviewers | |
rbintegrations, students | |
Added a Discord Integration option.
This allows Review Board to have an option of notifying Discord channels on
any Review Request changes. This commit also includes a refactoring of the
notify() function in SlackIntegration.
Added unit tests in discord/tests.py and they pass in RB3 virtual
environment. RB4 has two exceptions (please see below).Tested it manually in reviewboard 3.0.x and 4.0.x.
Issues that still need to be addressed:
Add assets (e.g. logos, trophies, etc.) at this location:
https://static.reviewboard.org/integration-assets/discord
It has the same problem as the Slack and Mattermost Integration:
two of the unit tests are complaining about this image here in RB4 -
/var/folders/f1/r8mpbfkd321804b47_tgytgw0000gn/T/rb-tests-EQOxMU/
static/rb/images/logo.png
Summary | Author | |
---|---|---|
HailanXyouknow |
Description | From | Last Updated |
---|---|---|
E261 at least two spaces before inline comment |
![]() |
|
E262 inline comment should start with '# ' |
![]() |
|
E261 at least two spaces before inline comment |
![]() |
|
E262 inline comment should start with '# ' |
![]() |
|
Please remove. |
|
|
TODO comment? |
|
|
Should this be Discord instead? |
|
|
Should this be Discord instead? |
|
|
Should this be Discord instead? |
|
|
Should this be Discord instead? |
|
|
It's more of a nitpick but 'mattemost' -> 'discord' |
|
|
Let's use double quotes for the quotation marks inside the string. |
|
|
Please add a blank line between these two. |
|
|
Maybe move the contents of this method back into the integration class? I don't see a good reason to have … |
|
|
Please use single quotes for these. |
|
|
Can we use a named logger here? At the top level of the file, add: logger = logging.getLogger(__name__) This can … |
|
|
There's an extra blank line here that can be removed. |
|
|
Please add a module docstring to the top of this file. |
|
|
Add a blank line between these two. |
|
|
Leftover debug output? |
|
|
This line isn't necessary. |
|
|
E302 expected 2 blank lines, found 1 |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
Instead of using the continuation character here, can we wrap this in parens? |
|
|
As per the coding style guide, I believe these imports should be moved down to a new group as they … |
|
|
Missing period here. |
|
|
Should be in alphabetical order. |
|
|
These imports should be moved down a group too. |
|
|
Let's switch the use of single- and double-quotes here: help_text=_('Your unique Discord webhook URL. This can be found at ' … |
|
|
These two lines should be swapped (alphabetical order). |
|
|
I think that lines 11 and 12 should be swapped so that they follow alphabetical order. |
![]() |
|
Two blank lines between imports and other code. |
|
|
I noticed that other review board files have 2 lines below the import statements. So I think that you should … |
![]() |
|
This should use ugettext_lazy, like the form does. |
|
|
Mind putting this in alphabetical order? Helps keep things organized as new settings are added. |
|
|
You can bump this to a modern date and time. |
|
|
This appears to be in the wrong spot. Should be further down, under event_name, and have , optional. |
|
|
Should have , optional |
|
|
Can you pass these as keyword arguments? Helps avoid issues as parameters get added, and keeps code readable. |
|
|
No need for the blank line here. We can convey this as one "paragraph", where we're building the initial state … |
|
|
Can you use '%s/slack' % config.... form? |
|
|
Can you set this to alphabetical order? |
|
|
If you use logger.exception, you won't need exc_info=True. |
|
|
Doesn't look like this function is doing that, at least not directly. Maybe state what it wraps, so it's clear … |
|
|
We don't need docstrings for setUp or tearDown. Sort of an exception to a rule. |
|
|
You can pass id=12321 to create_review_request() (in fact, it's important to do that — old code may not though). |
|
|
Same here with id=. |
|
|
No need to call this, since we already created it above. |
|
|
This will need a docstring. |
|
|
This can be a single statement. |
|
|
These all list , optional, but the arguments are all required in the method signature. |
|
|
Can you pass all these as keyword arguments? |
|
|
E201 whitespace after '{' |
![]() |
|
E202 whitespace before '}' |
![]() |
|
I have to add .encode('UTF-8) at the end of this line to get it to work for some reason. Running … |
|
|
Can you say "integration on Discord", so it's clear where they need to add this? |
|
|
This has two spellings of "Webhook": "Webhook" and "WebHook". Not your fault — we're inconsistent in places as well. I … |
|
|
"that that" -> "that" |
|
|
No space after the comma. |
|
|
lowercase: "utf-8". |
|
|
To check, is there a reason we use this User-Agent? If so, we should document this. If not, we shouldn't … |
|
|
Let's use "webhook". |
|
|
This is indented too far. |
|
|
Should be "dict". |
|
|
Since there's only one, how about just attachment? |
|
|
It's best to avoid one-line dictionaries with multiple keys, as it becomes harder to add to. Let's do: return { … |
|
rbintegrations/discord/tests.py (Diff revision 1) |
---|
Using Slack's format for Discord webhook messages. It looks cleaner
this way - we don't need to do weird things like translating hex
color code to decimal numbers
Description: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||||||||
Diff: |
Revision 2 (+662 -38) |
Summary: |
|
|||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||
Commits: |
|
|||||||||||||||||||||
Diff: |
Revision 3 (+3958 -38) |
Description: |
|
||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
I realized that the Integrations in extensions.py are in alphabetical order,
so this update is made to address this.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
Commits: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+3958 -38) |
|||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
rbintegrations/discord/tests.py (Diff revision 4) |
---|
It's more of a nitpick but 'mattemost' -> 'discord'
Address feedbacks
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+3960 -38) |
Looking pretty solid. Just some style-related comments.
rbintegrations/discord/forms.py (Diff revision 5) |
---|
Let's use double quotes for the quotation marks inside the string.
rbintegrations/discord/integration.py (Diff revision 5) |
---|
Maybe move the contents of this method back into the integration class? I don't see a good reason to have a separate top-level function here.
rbintegrations/discord/integration.py (Diff revision 5) |
---|
Can we use a named logger here? At the top level of the file, add:
logger = logging.getLogger(__name__)
This can then be
logger.debug(...)
rbintegrations/discord/integration.py (Diff revision 5) |
---|
There's an extra blank line here that can be removed.
rbintegrations/discord/tests.py (Diff revision 5) |
---|
Please add a module docstring to the top of this file.
Addressed David's feedback. Tested DiscordIntegration manually and reran the unit tests.
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+3844 -38) |
rbintegrations/discord/integration.py (Diff revision 6) |
---|
E128 continuation line under-indented for visual indent
rbintegrations/discord/integration.py (Diff revision 6) |
---|
E128 continuation line under-indented for visual indent
rbintegrations/discord/integration.py (Diff revision 6) |
---|
E128 continuation line under-indented for visual indent
rbintegrations/discord/integration.py (Diff revision 6) |
---|
E128 continuation line under-indented for visual indent
rbintegrations/discord/integration.py (Diff revision 6) |
---|
E128 continuation line under-indented for visual indent
rbintegrations/discord/integration.py (Diff revision 6) |
---|
E128 continuation line under-indented for visual indent
rbintegrations/discord/integration.py (Diff revision 6) |
---|
E128 continuation line under-indented for visual indent
Fix indentation problems
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+3844 -38) |
Looking very solid!
rbintegrations/discord/integration.py (Diff revision 7) |
---|
Instead of using the continuation character here, can we wrap this in parens?
Long string now in parentheses
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+3836 -38) |
Removed a line after Slack Colour bug Review Request is pushed.
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+3830 -38) |
Testing Done: |
|
---|
I don't know much about Discord Integration, so I just nit-picked on some styling issue.
rbintegrations/discord/forms.py (Diff revision 9) |
---|
As per the coding style guide, I believe these imports should be moved down to a new group as they are
Project imports
?
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+3830 -38) |
rbintegrations/discord/forms.py (Diff revision 10) |
---|
Let's switch the use of single- and double-quotes here:
help_text=_('Your unique Discord webhook URL. This can be found at ' 'the "Integration" page after selecting ' '"Server Settings".')
Address feedback
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+3830 -38) |
This looks great! Most of my comments are just small style nits for code/docs.
rbintegrations/discord/integration.py (Diff revision 11) |
---|
These two lines should be swapped (alphabetical order).
rbintegrations/discord/integration.py (Diff revision 11) |
---|
Two blank lines between imports and other code.
rbintegrations/discord/integration.py (Diff revision 11) |
---|
This should use
ugettext_lazy
, like the form does.
rbintegrations/discord/integration.py (Diff revision 11) |
---|
Mind putting this in alphabetical order? Helps keep things organized as new settings are added.
rbintegrations/discord/integration.py (Diff revision 11) |
---|
You can bump this to a modern date and time.
rbintegrations/discord/integration.py (Diff revision 11) |
---|
This appears to be in the wrong spot. Should be further down, under
event_name
, and have, optional
.
rbintegrations/discord/integration.py (Diff revision 11) |
---|
Can you pass these as keyword arguments? Helps avoid issues as parameters get added, and keeps code readable.
rbintegrations/discord/integration.py (Diff revision 11) |
---|
No need for the blank line here. We can convey this as one "paragraph", where we're building the initial state of the payload.
rbintegrations/discord/integration.py (Diff revision 11) |
---|
If you use
logger.exception
, you won't needexc_info=True
.
rbintegrations/discord/integration.py (Diff revision 11) |
---|
Doesn't look like this function is doing that, at least not directly. Maybe state what it wraps, so it's clear where to go for docs?
rbintegrations/discord/tests.py (Diff revision 11) |
---|
We don't need docstrings for
setUp
ortearDown
. Sort of an exception to a rule.
rbintegrations/discord/tests.py (Diff revision 11) |
---|
You can pass
id=12321
tocreate_review_request()
(in fact, it's important to do that — old code may not though).
rbintegrations/discord/tests.py (Diff revision 11) |
---|
No need to call this, since we already created it above.
rbintegrations/slack/integration.py (Diff revision 11) |
---|
These all list
, optional
, but the arguments are all required in the method signature.
I just noticed that Christian beat me to the review, so hopefully these aren't duplicates.
Otherwise, I manually tested the integration on my own discord server, and it works wonderfully.
I also ran the tests and all of them worked on RB3 but strangely there are 2 failing test cases on RB4.
However, these are already documented on the Notion documents.
rbintegrations/discord/integration.py (Diff revision 11) |
---|
I think that lines 11 and 12 should be swapped so that they follow alphabetical order.
rbintegrations/discord/integration.py (Diff revision 11) |
---|
I noticed that other review board files have 2 lines below the import statements. So I think that you should have 2 lines after the last import statement for your
logger = logging.getLogger(__name__)
and then 2 lines below before the class definition.
Address feedbacks from Christian and Jacob
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 12 (+3860 -60) |
Fix spacing issue
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 13 (+3862 -60) |
rbintegrations/discord/integration.py (Diff revision 13) |
---|
I have to add .encode('UTF-8) at the end of this line to get it to work for some reason. Running into this issue when I try to get a message to show up on discord:
TypeError: POST data should be bytes, an iterable of bytes, or a file object. It cannot be of type str.
Other than that, tested it and works well!
Addressed Ruonan's feedback.
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 14 (+3862 -60) |
Just a few small things, and then this will land :)
rbintegrations/discord/forms.py (Diff revision 14) |
---|
Can you say "integration on Discord", so it's clear where they need to add this?
rbintegrations/discord/forms.py (Diff revision 14) |
---|
This has two spellings of "Webhook": "Webhook" and "WebHook". Not your fault — we're inconsistent in places as well. I think we should keep any non-title versions "webhook" (no capitalizations), as this is more standard these days.
rbintegrations/discord/integration.py (Diff revision 14) |
---|
To check, is there a reason we use this User-Agent? If so, we should document this. If not, we shouldn't include it.
rbintegrations/slack/integration.py (Diff revision 14) |
---|
Since there's only one, how about just
attachment
?
rbintegrations/slack/integration.py (Diff revision 14) |
---|
It's best to avoid one-line dictionaries with multiple keys, as it becomes harder to add to. Let's do:
return { 'attachments': [attachments], 'icon_url': integration.LOGO_URL, }
Also note the alphabetical order.
Address Christian's feedback
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 15 (+3872 -60) |