-
Notifications
You must be signed in to change notification settings - Fork 581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use PythonImportRequirement #1669
base: master
Are you sure you want to change the base?
Conversation
The commit 1 replaces
|
I think we will need to amend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pep8 and autoflake look great. now you just need to do the same for the rest of the bears.
(We do not need extra hooks like import_prerequisites
, there are other core enhancements which will mean these imports are done automatically for the bear, but that will be in 0.12)
bears/general/InvalidLinkBear.py
Outdated
@@ -19,7 +20,7 @@ | |||
class InvalidLinkBear(LocalBear): | |||
DEFAULT_TIMEOUT = 15 | |||
LANGUAGES = {'All'} | |||
REQUIREMENTS = {PipRequirement('requests', '2.12')} | |||
REQUIREMENTS = {PythonImportRequirement('requests', '2.12')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InvalidLinkBear
doesnt need PythonImportRequirement
The only second party import is requests, which is also a dependency of coala.
So this bear doesnt have any extra dependencies.
bears/python/YapfBear.py
Outdated
@@ -12,7 +13,11 @@ | |||
class YapfBear(LocalBear): | |||
LANGUAGES = {'Python', 'Python 2', 'Python 3'} | |||
AUTHORS = {'The coala developers'} | |||
REQUIREMENTS = {PipRequirement('yapf', '0.14.0')} | |||
REQUIREMENTS = {PythonImportRequirement( | |||
'yapf', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow, ugly.
move PythonImportRequirement(
down onto this line
|
@@ -5,7 +5,8 @@ | |||
from safety import safety |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this one? What was the problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has to do something with lines like these in tests:
with mock.patch(
'bears.python.requirements.PySafetyBear.safety.check',
return_value=[],
) as check:
when I remove from safety import safety
in the Bear. All test fail.
Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again! |
1 similar comment
Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again! |
@@ -46,11 +47,13 @@ def _seperate_imports(file): | |||
return import_stmts | |||
|
|||
def _get_diff(self): | |||
isort = list(self.__class__.REQUIREMENTS)[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E128 continuation line under-indented for visual indent'
PycodestyleBear (E128), severity NORMAL, section autopep8
.
@@ -12,7 +11,9 @@ | |||
class PyImportSortBear(LocalBear): | |||
|
|||
LANGUAGES = {'Python', 'Python 3', 'Python 2'} | |||
REQUIREMENTS = {PipRequirement('isort', '4.2')} | |||
REQUIREMENTS = {PythonImportRequirement('isort', | |||
'4.2', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E128 continuation line under-indented for visual indent'
PycodestyleBear (E128), severity NORMAL, section autopep8
.
@@ -46,11 +47,13 @@ def _seperate_imports(file): | |||
return import_stmts | |||
|
|||
def _get_diff(self): | |||
isort = list(self.__class__.REQUIREMENTS)[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -71,7 +71,7 @@
return diff
else:
sort_imports = isort.SortImports(file_contents=''.join(self.file),
- **self.isort_settings)
+ **self.isort_settings)
new_file = tuple(sort_imports.output.splitlines(True))
if new_file != tuple(self.file):
@@ -12,7 +11,9 @@ | |||
class PyImportSortBear(LocalBear): | |||
|
|||
LANGUAGES = {'Python', 'Python 3', 'Python 2'} | |||
REQUIREMENTS = {PipRequirement('isort', '4.2')} | |||
REQUIREMENTS = {PythonImportRequirement('isort', | |||
'4.2', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -54,8 +54,8 @@
sorted_imps = []
for units in import_stmts:
sort_imports = isort.SortImports(file_contents=''.
- join([x[1] for x in units]),
- **self.isort_settings)
+ join([x[1] for x in units]),
+ **self.isort_settings)
sort_imports = sort_imports.output.splitlines(True)
sorted_imps.append((units, sort_imports))
8a2fd89
to
da223bf
Compare
Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again! |
@@ -53,8 +55,12 @@ def run(self, | |||
# language_check being there. | |||
from language_check import LanguageTool, correct | |||
|
|||
for requirement in list(self.__class__.REQUIREMENTS): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this a little less ugly, I have started https://gitlab.com/coala/package_manager/merge_requests/91 .
We would use it like REQUIREMENTS = AddressableRequirementSet([PythonImportRequirement(...), ...])
Let me know what you think. It isnt polished yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://gitlab.com/coala/package_manager/merge_requests/91 has landed.
I'll get it released and available for you today.
But you can already experiment with it (e.g. by changing the versions manually).
I'd love to see your patch merged this weekend if possible, so I can finish #1000.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jayvdb has this release happened yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternatively we can use a list instead of a set for the requirement or make the pythonimportrequirement a class level constant, that'd be far simpler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coala core is using depman master, so this should now be ready to roll.
Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again! |
Oh no!! I messed up. |
Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again! |
5 similar comments
Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again! |
Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again! |
Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again! |
Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again! |
Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again! |
bears/python/PEP8Bear.py
Outdated
@@ -39,6 +37,8 @@ def run(self, filename, file, | |||
:param local_pep8_config: Set to true if autopep8 should use a config | |||
file as if run normally from this directory. | |||
""" | |||
autopep8 = list(self.__class__.REQUIREMENTS)[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dangerous, somebody might add something to the set and out of a sudden it might fail to work with some interpreters or completely nondeterministic
good stuff, I really think we should have the import requirements at class level and put those into sets and use them directly, that's much more simpler and obvious than an AdressableRequirements class IMO |
Using PythonImportRequirement instead of PipRequirement for these bears which import dependencies directly in bear: - GitCommitBear - LanguageToolBear - PEP8Bear - PEP8NotebookBear - PyCommentedCodeBear - PyImportSortBear - PyUnusedCodeBear - PyromaBear - PySafetyBear - RadonBear - reSTLintBear - RuboCopBear - SCSSLintBear - YAMLLintBear - YapfBear Related to coala#1655
Fixes some typos. Removes direct import in these bears: - PEP8Bear - PEP8NotebookBear - PyCommentedCodeBear - PyImportSortBear - PyUnusedCodeBear - PyromaBear - RadonBear - YapfBear - LanguageToolBear - reSTLintBear - RuboCopBear - SCSSLintBear - GitCommitBear - YAMLLintBear Closes coala#1655
Using PythonImportRequirement instead of PipRequirement for bears which
import directly into the bear.
Related to #1655
For short term contributors: we understand that getting your commits well
defined like we require is a hard task and takes some learning. If you
look to help without wanting to contribute long term there's no need
for you to learn this. Just drop us a message and we'll take care of brushing
up your stuff for merge!
Checklist
them.
individually. It is not sufficient to have "fixup commits" on your PR,
our bot will still report the issues for the previous commit.) You will
likely receive a lot of bot comments and build failures if coala does not
pass on every single commit!
After you submit your pull request, DO NOT click the 'Update Branch' button.
When asked for a rebase, consult coala.io/rebase
instead.
Please consider helping us by reviewing other peoples pull requests as well:
cobot mark wip <URL>
to get it outof the review queue.
The more you review, the more your score will grow at coala.io and we will
review your PRs faster!