-
Notifications
You must be signed in to change notification settings - Fork 28
Pre-Release: add precommit-hook configuration for linters, doc styles #68
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #68 +/- ##
==========================================
- Coverage 97.79% 97.78% -0.02%
==========================================
Files 10 10
Lines 454 451 -3
==========================================
- Hits 444 441 -3
Misses 10 10
|
|
Hi @kmike @Gallaecio , I've updated this previously draft-PR so it can be included in the upcoming release. :) |
|
|
||
| import scrapy | ||
|
|
||
| from example.autoextract import ProductPage |
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.
I wonder if we can make example imports relative, the way they are placed by black (?) look a bit weird otherwise. This applies to other files here as well.
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.
Hi @kmike , I'm afraid I don't follow as this line is using relative imports. Perhaps what you meant was that the linter (i.e. https://github.com/PyCQA/isort) moved this line from down below to up here?
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.
I think he meant from ...autoextract import ProductPage (I might have gotten the number of dots wrong 🙂).
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.
Perhaps what you meant was that the linter (i.e. https://github.com/PyCQA/isort) moved this line from down below to up here?
sorry, that was exactly my concern
I think he meant from ...autoextract import ProductPage
yes, that's what I meant :) Sorry folks for an unclear message. It's not a big deal.
pyproject.toml
Outdated
| @@ -0,0 +1,6 @@ | |||
| [tool.black] | |||
| line-length = 120 | |||
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.
I know this is bikeshedding, but maybe we should stick to either 79 or 88; those two values seem to be the most common standards.
https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#line-length motivation seems reasonable to me.
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.
I have a personal preference of 79, but I gave up imposing it in shared repositories a long time ago, so +1 to any choice.
And I am not sure 79 and 88 are the most common, PyCharm is probably the most popular Python IDE, and it enforces 120 by default, which probably has an effect on users.
PS: I also have a personal preference for blue over black, but I won’t push for that here either :)
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.
I don't have a strong opinion here. Black's default of 88 seems reasonable. Updated it to use line length of 88.
TODO before releasing a new version
tox -e lintersto fix all formatting issues and address any linting issues.git-blame-ignore-revsfile