flake8
-
rbintegrations/microsoftteams/test.py (Diff revision 1) Show all issues -
-
-
Review Request #11227 — Created Oct. 16, 2020 and updated
Information | |
---|---|
hailan | |
rbintegrations | |
master | |
Reviewers | |
rbintegrations, students | |
Added the Microsoft Teams Integration Option. Like the existing Slack
Integration option, this integration will post a message to an MS Teams chat
channel after a review request has been updated.MS Teams icons are acquired from Adaptive Cards' main webpage:
https://adaptivecards.io/
Unit tests passed and manually tested it in RB3.0 and 4.0.
Summary | Author |
---|---|
HailanXyouknow |
Description | From | Last Updated |
---|---|---|
How about renaming the module from microsoftteams to msteams? It'll be shorter and imports won't have to wrap. |
|
|
E501 line too long (172 > 79 characters) |
![]() |
|
E501 line too long (210 > 79 characters) |
![]() |
|
E262 inline comment should start with '# ' |
![]() |
|
W292 no newline at end of file |
![]() |
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
E266 too many leading '#' for block comment |
![]() |
|
E231 missing whitespace after ':' |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
E303 too many blank lines (3) |
![]() |
|
W291 trailing whitespace |
![]() |
|
E999 SyntaxError: invalid syntax |
![]() |
|
E231 missing whitespace after ':' |
![]() |
|
E262 inline comment should start with '# ' |
![]() |
|
W291 trailing whitespace |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
E501 line too long (210 > 79 characters) |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
W291 trailing whitespace |
![]() |
|
E122 continuation line missing indentation or outdented |
![]() |
|
E122 continuation line missing indentation or outdented |
![]() |
|
E122 continuation line missing indentation or outdented |
![]() |
|
E122 continuation line missing indentation or outdented |
![]() |
|
E501 line too long (115 > 79 characters) |
![]() |
|
E501 line too long (91 > 79 characters) |
![]() |
|
E122 continuation line missing indentation or outdented |
![]() |
|
E262 inline comment should start with '# ' |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
E501 line too long (86 > 79 characters) |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
E501 line too long (83 > 79 characters) |
![]() |
|
Should swap lines 10 and 13, so that they are in alphabetical order. |
![]() |
|
logger = logging.getLogger(__name__) should have 2 lines before and 2 lines after. |
![]() |
|
This could also be if, elif, else. Not sure what the mentors prefer. |
![]() |
|
not a must change but could also use a switch-case statement |
|
|
Comment should have a period. |
![]() |
|
Comment should have a period. |
![]() |
|
I think it is good practice to include the alt attribute for img tags in html. |
![]() |
|
Looks like a debug statement, we should remove these. |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
Please add a module docstring. |
|
|
This isn't actually used (the call site is using the top-level function). However, I think what I'd do is move … |
|
|
What's left to do here? |
|
|
When we wrap the docstring, we put the final """ on its own line. I think I marked all the … |
|
|
What's left to do here? |
|
|
When we wrap the docstring, we put the final """ on its own line. |
|
|
When we wrap the docstring, we put the final """ on its own line. |
|
|
When we wrap the docstring, we put the final """ on its own line. |
|
|
When we wrap the docstring, we put the final """ on its own line. |
|
|
There's an extra "!" here. |
|
|
When we wrap the docstring, we put the final """ on its own line. |
|
|
When we wrap the docstring, we put the final """ on its own line. |
|
|
When we wrap the docstring, we put the final """ on its own line. |
|
|
This should end in a period (since it's not a test case docstring). Please also add "Args" and "Returns" sections. |
|
|
Blank line after this. We ahould also just have a variable for the emoji name and code, rather than a … |
|
|
Blank line between these. |
|
|
No need for the parens around the ... is .... |
|
|
We should use format_html(), since it's safer (the arguments going in will be HTML-escaped). |
|
|
This should use .copy() instead of dict(...). |
|
|
We standardize on lowercase utf-8. |
|
|
Docstring summaries must be one line without wrapping. |
|
rbintegrations/microsoftteams/test.py (Diff revision 1) |
---|
Started to implement changes in integration.py
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+686) |
rbintegrations/microsoftteams/integration.py (Diff revision 2) |
---|
E266 too many leading '#' for block comment
Finished adding all fields to payload.
Manually tested it: looks fine on desktop version of MS Teams.
On the mobile app it doesn't show the attached image_url or thumb_url.
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+1588) |
rbintegrations/microsoftteams/integration0.py (Diff revision 3) |
---|
E501 line too long (81 > 79 characters)
rbintegrations/microsoftteams/integration0.py (Diff revision 3) |
---|
E501 line too long (80 > 79 characters)
rbintegrations/microsoftteams/integration0.py (Diff revision 3) |
---|
E501 line too long (80 > 79 characters)
rbintegrations/microsoftteams/test.py (Diff revision 3) |
---|
E122 continuation line missing indentation or outdented
rbintegrations/microsoftteams/test.py (Diff revision 3) |
---|
E122 continuation line missing indentation or outdented
rbintegrations/microsoftteams/test.py (Diff revision 3) |
---|
E122 continuation line missing indentation or outdented
rbintegrations/microsoftteams/test.py (Diff revision 3) |
---|
E122 continuation line missing indentation or outdented
rbintegrations/microsoftteams/test.py (Diff revision 3) |
---|
E122 continuation line missing indentation or outdented
Clean up integrations.py
Description: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||
Diff: |
Revision 4 (+462) |
||||||||||||
Removed Files: |
|||||||||||||
Added Files: |
rbintegrations/microsoftteams/integration.py (Diff revision 4) |
---|
E127 continuation line over-indented for visual indent
Unit tests in tests.py
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+3430) |
Update tests.py
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+3528) |
rbintegrations/microsoftteams/integration.py (Diff revision 6) |
---|
E501 line too long (83 > 79 characters)
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+3532) |
rbintegrations/microsoftteams/integration.py (Diff revision 7) |
---|
not a must change but could also use a switch-case statement
I manually tested the integration, it works great!
I also ran the automated tests and I had similar issues that the Discord Integration had, which was the 2 test failures.
Otherwise, everything looks good to me, except for some styling. I did my best to review, although, I am not an expert on the integrations side of Review Board.
rbintegrations/microsoftteams/integration.py (Diff revision 7) |
---|
Should swap lines 10 and 13, so that they are in alphabetical order.
rbintegrations/microsoftteams/integration.py (Diff revision 7) |
---|
logger = logging.getLogger(__name__)
should have 2 lines before and 2 lines after.
rbintegrations/microsoftteams/integration.py (Diff revision 7) |
---|
This could also be
if
,elif
,else
. Not sure what the mentors prefer.
rbintegrations/microsoftteams/integration.py (Diff revision 7) |
---|
I think it is good practice to include the
alt
attribute forimg
tags in html.
rbintegrations/microsoftteams/integration.py (Diff revision 7) |
---|
Looks like a debug statement, we should remove these.
Address Ruonan and Jacob's feedbacks
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+3526) |
Address white space
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+3526) |
Updated forms.py - the old help text is difficult to read
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+3524) |
Capitalize the word Webhook
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+3524) |
rbintegrations/microsoftteams/integration.py (Diff revision 11) |
---|
This isn't actually used (the call site is using the top-level function). However, I think what I'd do is move the contents of that function into this one, and then change the call site to use
self.format_link
.
rbintegrations/microsoftteams/tests.py (Diff revision 11) |
---|
When we wrap the docstring, we put the final """ on its own line. I think I marked all the places where this happens but please go through and check all the test cases.
rbintegrations/microsoftteams/tests.py (Diff revision 11) |
---|
When we wrap the docstring, we put the final """ on its own line.
rbintegrations/microsoftteams/tests.py (Diff revision 11) |
---|
When we wrap the docstring, we put the final """ on its own line.
rbintegrations/microsoftteams/tests.py (Diff revision 11) |
---|
When we wrap the docstring, we put the final """ on its own line.
rbintegrations/microsoftteams/tests.py (Diff revision 11) |
---|
When we wrap the docstring, we put the final """ on its own line.
rbintegrations/microsoftteams/tests.py (Diff revision 11) |
---|
When we wrap the docstring, we put the final """ on its own line.
rbintegrations/microsoftteams/tests.py (Diff revision 11) |
---|
When we wrap the docstring, we put the final """ on its own line.
rbintegrations/microsoftteams/tests.py (Diff revision 11) |
---|
When we wrap the docstring, we put the final """ on its own line.
rbintegrations/microsoftteams/tests.py (Diff revision 11) |
---|
This should end in a period (since it's not a test case docstring). Please also add "Args" and "Returns" sections.
Addressed David's feedback.
Description: |
|
||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||||||||||||||
Diff: |
Revision 12 (+3504) |
Added `.encode('UTF-8')` to our request.
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 13 (+3504) |
This is great! Thanks for working on this :)
The semester is over, and you're probably ready for a break. We're ready to release very soon, so it's completely up to you whether you want to take on the review feedback here or leave it up to us.
How about renaming the module from
microsoftteams
tomsteams
? It'll be shorter and imports won't have to wrap.
rbintegrations/microsoftteams/integration.py (Diff revision 13) |
---|
Blank line after this.
We ahould also just have a variable for the emoji name and code, rather than a dictionary, since they're local. It'll be faster to access a local variable than a dictionary key, and it'll look cleaner.
rbintegrations/microsoftteams/integration.py (Diff revision 13) |
---|
No need for the parens around the
... is ...
.
rbintegrations/microsoftteams/integration.py (Diff revision 13) |
---|
We should use
format_html()
, since it's safer (the arguments going in will be HTML-escaped).
rbintegrations/microsoftteams/integration.py (Diff revision 13) |
---|
This should use
.copy()
instead ofdict(...)
.
rbintegrations/microsoftteams/integration.py (Diff revision 13) |
---|
Docstring summaries must be one line without wrapping.
Address feedbacks
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 14 (+3498) |