Apply coala patches #41
Conversation
There was a problem hiding this comment.
IMO, this patch should be held back in the queue, and used as a guide to help us with code reviews.
Many parts of this will be rewritten during GCI, especially data being moved out into data files, and as part of that we could ask that new code follows the new coding standards.
manu-chroma
force-pushed
the
apply-coala-patches
branch
5 times, most recently
from
dc57180 to
249e6c1
Compare
|
@jayvdb Ref to: https://travis-ci.org/manu-chroma/username-availability-checker/builds/321837314#L846 Update: @biscuitsnake is again working on her patch. This will cause conflicts Your call John on how we should handle this one 😅 |
no. see https://github.com/coala/coala-bears/blob/master/bears/general/LineLengthBear.py#L23 |
cli.py
Outdated
| sites = list(map(lambda x: '{}.com'.format(x), | ||
| "facebook twitter instagram github youtube soundcloud tumblr".split())) | ||
| sites = list(map(lambda x: '{}.com'.format(x), | ||
| 'facebook twitter instagram \ |
There was a problem hiding this comment.
this list should be derived from the yaml, as the list is now out of date. especially, wtf does youtube come from? :P maybe cli.py isnt too useful if nobody has noticed that youtube results here are not sensible.
anyway, as a general rule the continuation marker \ should be avoided. Use of it is almost always unnecessary, and it is rarely supported properly by linters because it is a override that breaks most of the rules of the language.
In this case, consecutive strings are automatically concatenated if they are inside any multiline context.
i.e.
a = ('facebook twitter instagram '
'github youtube soundcloud tumblr')There was a problem hiding this comment.
Fixing the linting errors for now.
I realise how badly cli.py is broken (also, we used to support YouTube back in the day ) :P
I will open an issue to fix it separately. It is low priority and we can choose to actually fix or remove the file for later.
username_api.py
Outdated
|
|
||
| def check_format(website, username): | ||
| """Check the format of a username depending on the website""" | ||
| '""Check the format of a username depending on the website""' |
There was a problem hiding this comment.
this is a horrible bug in coala. ^ is not a docstring.
use
"""Check the format of a username depending on the website.
"""
.coafile
Outdated
| @@ -30,7 +30,7 @@ use_spaces = true | |||
| files = templates/*.html | |||
| bears = LineLengthBear, SpaceConsistencyBear | |||
| use_spaces = true | |||
| max_line_length = 100 | |||
| max_line_length = 700 | |||
There was a problem hiding this comment.
too high to be useful.
need to find ways to bring that down, configuring the bears, excluding specific files, etc.
tests/test_username_api.py
Outdated
| import string | ||
|
|
||
| from parameterized import parameterized | ||
| import pytest | ||
| import os.path |
There was a problem hiding this comment.
these were recently corrected in master. this is probably a merge mistake
tests/test_username_api.py
Outdated
|
|
||
| message = None | ||
| if expected != actual: | ||
| message = 'The provided available username ({}) ' \ |
There was a problem hiding this comment.
\ is an abomination :P
string concatenation doesnt need line continuation.
increasing the line length for python would be reasonable IMO.
There was a problem hiding this comment.
I was very strictly trying to adhere to PEP8 limit :P
tests/test_username_api.py
Outdated
| json_resp = json.loads(resp.get_data().decode()) | ||
| url = username_api.get_profile_url(website, username) | ||
| assert { | ||
| 'possible': False, |
There was a problem hiding this comment.
personally, I prefer the existing layout.
assert {
'possible': False,
'url': url
} == json_respor its inverse
assert json_resp == {
'possible': False,
'url': url
}You've also kept that style at https://github.com/manu-chroma/username-availability-checker/pull/41/files#diff-86cae0060e15d8121c1f9029f69f7517R82
tests/test_username_api.py
Outdated
| url = username_api.get_profile_url(website, username) | ||
| assert { | ||
| 'possible': False, | ||
| 'url': url |
There was a problem hiding this comment.
you may like to impose trailing commas in this patch, but doing that would change this line, which means git blame -w will be broken by this patch.
tests/test_get_avatar.py
Outdated
| pytest.skip('website not supported') | ||
| link = username_api.check_username(website, user)['avatar'] | ||
| response = r.get(link) | ||
| assert(response.headers.get('content-type', '').startswith('image/') or |
There was a problem hiding this comment.
Unrelated to your patch, but good to include now as it is another whitespace only change ...
assert(.. looks like a function call.
instead, adding a space can help ensure people see a function call was not intended
assert (..
manu-chroma
force-pushed
the
apply-coala-patches
branch
from
249e6c1 to
4f81a48
Compare
Also, relax restriction on `requests`
We've a massive svg line in the code and few other lines which go upto 300. This can be addressed in the future.
Fix asset paths in `templates/` accordingly
manu-chroma
force-pushed
the
apply-coala-patches
branch
2 times, most recently
from
31933ac to
6782f17
Compare
|
@jayvdb :) |
This patch is for *.py files. Includes: - consistant indentation for all files - patches from PEP8Bear, PycodestyleBear - PyCharm's inspect code analysis - other bears in .coafile - optimise imports
To not cause version conflict when installing coala.
Ignore long links in CSS and ignore svg path tags in HMTL. This helps in reducing HTML line length down to a more meaningful number, to be useful.
manu-chroma
force-pushed
the
apply-coala-patches
branch
from
6782f17 to
ba491c0
Compare
No description provided.