PyImportSortBear.py: Show incorrectly sorted imports #886

wimpykid26 · 2016-10-10T19:42:45Z

This will ensure that coala is
able to display the import statements
that are sorted incorrectly.

Closes #397

gitmate-bot · 2016-10-10T19:43:00Z

Thanks for your contribution!

Reviewing pull requests take really a lot of time and we're all volunteers. Please make sure you go through the following check list and complete them all before pinging someone for a review.

I have rebased properly. Please see this for a tutorial.
I have gone through the commit guidelines and I've followed them:
- I have followed the guidelines for the commit shortlog.
- I have followed the guidelines for the commit body.
- I have included the issue URL.
- I have used the Fixes keyword if the commit fixes a real bug and the Closes keyword if it adds a feature/enhancement. See more here.
I have run all the tests and they all pass. That includes that all CI (below) is green - if GitMate issues appear you will have to amend your commits, pushing new commits on top is not sufficient!

As you learn things over your Pull Request please help others on the chat and on PRs to get their stuff right as well!

gitmate-bot · 2016-10-10T19:43:17Z

Comment on 30cbdbc.

Shortlog of HEAD commit is 2 character(s) longer than the limit (52 > 50).

GitCommitBear, severity NORMAL, section commit.

wimpykid26 · 2016-10-10T19:46:43Z

@sils could you please review ?

sils · 2016-10-10T19:47:47Z

#886 (comment) ?

sils · 2016-10-10T19:48:08Z

build's red

adtac · 2016-10-11T12:45:23Z

tests failing; marking as WIP till then

adtac

Okay I don't know about the Diff class enough to review the rest, sorry.

adtac · 2016-10-12T09:38:29Z

bears/python/PyImportSortBear.py

@@ -182,7 +184,19 @@ def run(self, filename, file,

        if new_file != tuple(file):
            diff = Diff.from_string_arrays(file, new_file)
+            all_imports = ''
+            diff = Diff.from_string_arrays(file, new_file)


Line 188 and Line 186 do the same thing

wimpykid26 · 2016-10-12T19:01:48Z

Could anyone please review?

sils · 2016-10-12T19:30:09Z

@wimpykid26 please help us reviewing other PRs so you can get faster reviews. Also note that on many other big organizations you have average waiting times of weeks to months for patches.

Makman2 · 2016-10-12T20:26:37Z

bears/python/PyImportSortBear.py

@@ -182,7 +184,18 @@ def run(self, filename, file,

        if new_file != tuple(file):
            diff = Diff.from_string_arrays(file, new_file)
+            all_imports = ''
+            for line_nr in sorted(diff._changes):


_changes is a "protected" member, you shouldn't access it^^

there's probably another member that allows you to iterate over the changes 👍

@Makman2 Thanks for pointing that out :). Should I use the range function to get the range of the diff and then use the _get_changes function for parsing?

Makman2 · 2016-10-12T20:26:39Z

bears/python/PyImportSortBear.py

+                    list_of_imports[0].lstrip()
+                    list_of_imports.sort()
+                    string_of_imports = ' '.join(list_of_imports)
+                    all_imports = all_imports + " " + string_of_imports


you can do this a bit more elegant :)
Make all_imports above to a list and change this line to:

all_imports = [] for line_nr in sorted(diff._changes): #.... all_imports.append(string_of_imports)

ah and below doing

"Imports " + " ".join(all_imports) + " are sorted incorrectly"

or you could use format():

"Imports {} are sorted incorrectly.".format(" ".join(...))

Makman2 · 2016-10-12T20:30:16Z

bears/python/PyImportSortBear.py

@@ -182,7 +184,18 @@ def run(self, filename, file,

        if new_file != tuple(file):
            diff = Diff.from_string_arrays(file, new_file)
+            all_imports = ''
+            for line_nr in sorted(diff._changes):
+                linediff = diff._file[max(line_nr - 1, 0)]


here too, please don't access _file, I think we have actually a file property, don't we?

Makman2 · 2016-10-12T20:30:50Z

bears/python/PyImportSortBear.py

+            for line_nr in sorted(diff._changes):
+                linediff = diff._file[max(line_nr - 1, 0)]
+                list_of_imports = re.split(', +',
+                                           linediff.rstrip())


fits into a single line (improves readability because of the comma inside the quotes, that's kinda confusing :3)

Makman2 · 2016-10-12T20:32:36Z

bears/python/PyImportSortBear.py

            yield Result(self,
-                         "Imports can be sorted.",
+                         "Imports" + all_imports + " are sorted incorrectly",


Missing space after Imports and missing period at end of message

Makman2 · 2016-10-12T20:34:57Z

bears/python/PyImportSortBear.py

+                list_of_imports = re.split(', +',
+                                           linediff.rstrip())
+                if list_of_imports[0].startswith('import'):
+                    list_of_imports[0] = list_of_imports[0].lstrip('import')


lstrip doesn't work like this, for example this string
importimoopX
would expand to
X
as lstrip strips each character away, not a complete string in the given order. You can use str.replace or if you need additional whitespace stripping, you can use regexes and their replacement functions

Makman2 · 2016-10-12T20:37:00Z

unack 151cb89

gitmate-bot · 2016-10-15T14:19:03Z

bears/python/PyImportSortBear.py

@@ -182,7 +184,19 @@ def run(self, filename, file,

        if new_file != tuple(file):
            diff = Diff.from_string_arrays(file, new_file)
+            all_imports = []
+            for line_nr in range(diff.range(filename).start.line, diff.range(filename).end.line + 1):


Line is longer than allowed. (101 > 79)

LineLengthBear, severity NORMAL, section linelength.

gitmate-bot · 2016-10-15T14:22:50Z

bears/python/PyImportSortBear.py

@@ -182,7 +184,20 @@ def run(self, filename, file,

        if new_file != tuple(file):
            diff = Diff.from_string_arrays(file, new_file)
+            all_imports = []
+            for line_nr in range(diff.range(filename).start.line,
+            diff.range(filename).end.line + 1):


The code does not comply to PEP8.

PEP8Bear, severity NORMAL, section autopep8.

The issue can be fixed by applying the following patch:

--- a/bears/python/PyImportSortBear.py +++ b/bears/python/PyImportSortBear.py @@ -186,7 +186,7 @@ diff = Diff.from_string_arrays(file, new_file) all_imports = [] for line_nr in range(diff.range(filename).start.line, - diff.range(filename).end.line + 1): + diff.range(filename).end.line + 1): if(diff._get_change(line_nr).change is not False): line_diff = diff._get_change(line_nr).change[0] list_of_imports = re.split(', +', line_diff.rstrip())

gitmate-bot · 2017-01-16T17:05:54Z