tests: Remove unused import statements #1143
Conversation
|
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.
As you learn things over your Pull Request please help others on the chat and on PRs to get their stuff right as well! |
|
Do not use the GitHub rebase feature. See https://github.com/coala/coala-bears/pull/1143/commits |
Techievena
force-pushed
the
issue#1126
branch
from
4cbd92a to
ba19f2f
Compare
| @@ -1,5 +1,4 @@ | |||
| import sys | |||
| import platform # Unused import | |||
There was a problem hiding this comment.
I think everything in tests/python/test_files/ shouldnt be modified - they are intentionally bad syntax
There was a problem hiding this comment.
Sorry @jayvdb I am unable to figure out what changes I have to make in this
There was a problem hiding this comment.
Restore this line as it was. We'll find a solution for this line in coala/coala#3066 , after your patch is merged.
There was a problem hiding this comment.
You havent restored this line exactly.
There was a problem hiding this comment.
Sorry I'd do that I thought It'd be same if I rewrite the line
| @@ -6,13 +6,6 @@ | |||
| from tests.BearTestHelper import generate_skip_decorator | |||
| from tests.LocalBearTestHelper import verify_local_bear | |||
|
|
|||
| try: | |||
There was a problem hiding this comment.
this small block needs to stay here. See the voodoo in https://github.com/coala/coala-bears/pull/1135/files to work around this problem.
ie. use
import language_check
import guess_language
language_check
guess_language
Techievena
force-pushed
the
issue#1126
branch
from
20c0f82 to
a809e9e
Compare
Techievena
force-pushed
the
issue#1126
branch
from
a809e9e to
eceeb35
Compare
| @@ -9,10 +9,11 @@ | |||
| try: | |||
| import language_check | |||
| import guess_language | |||
| language_check | |||
There was a problem hiding this comment.
you had to remove these right? not just the import keyword but everything?
There was a problem hiding this comment.
@jayvdb are we sure we want to remove these? it looks like the bear uses these to determine wether to skip the test?
There was a problem hiding this comment.
use a coala ignore statement here?
There was a problem hiding this comment.
this approach adds two lines, and works with pyflakes natively.
your solution adds or amends two lines, and only works within coala.
I'm not seeing the benefit, especially not work holding up this patch and potentially conflicting with other patches incoming. If necessary we can switch later.
|
@Techievena you didnt remove this: |
|
@abhsag24 I didn't remove [tests/python/test_files/pylint_test.py:2: 'platform' imported but unused] as @jayvdb commented it as a bad syntax and it will be handled in a different issue. So he asked me to restore it. |
|
@jayvdb what about eceeb35#r92918230 ? |
|
ack eceeb35 |
|
I think it needs a rebase, but I cant verify that easily atm |
|
yep it does
|
|
Do I have to modify this pull request??? (Rebase it????) |
|
Yup. rebase it pls |
Remove the unused imports in tests directory, to help prepare for coala/coala#3066 Fixes coala#1126
Techievena
force-pushed
the
issue#1126
branch
from
eceeb35 to
ff17210
Compare
|
ack ff17210 |
|
@rultor merge |
|
@Techievena @jayvdb is there any bear we can activate to prevent this from happening in the future? |
Remove the unused imports in tests directory,
to help prepare for coala/coala#3066
Fixes #1126