-
Notifications
You must be signed in to change notification settings - Fork 163
added pre-commit hooks #173
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
Wanted to add black as well but it is not supported in python3.5. 😞 |
Why did you change single-quotes to double-quotes? |
Sorry about that, so when I started implementing the pre-commits, I included black formatter but it was not supported on python3.4 as well as 3.5 so had to remove it. It was black that formatted the single quotes to double quotes. I can undo the changes if its necessary? |
Please do, I personally like single quotes more and see no reason for changing those. |
I have reverted back the changes that black had made. Please tell me if there is anything out of place, I will be happy to change it. |
@kvesteri Could you may be please have a look |
You forgot to revert them in |
setup.py
Outdated
|
||
PY3 = sys.version_info[0] == 3 | ||
HERE = os.path.dirname(os.path.abspath(__file__)) | ||
|
||
|
||
def get_version(): |
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.
Why did you remove this function? I find this function really convenient in a sense that I don't have to remember to update the version number in two different places. If this is removed then I'd like to have a viable alternative for this rather than hard-coding the version number in this file as well.
Reverted back! Also @kvesteri why we are still not using Github actions? |
@@ -0,0 +1,17 @@ | |||
[isort] | |||
known_first_party=sqlalchemy_utils,tests | |||
line_length=88 |
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.
Why did you change this line length from 79 to 88? In general I'd prefer this PR kept everything the same as it was and just add the pre-commit hooks.
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.
Sure, I will change it back, but 88 seems to be the default for black formatter and I thought that some time in the future we will introduce black as the formatter so I changed it. 88 seems to be the sweet spot. Anyways I will change it.
include_trailing_comma=True | ||
|
||
[flake8] | ||
ignore = D10,E203,E501,W503,E231,E265,W504,E122,E121 |
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.
Is there a reason for adding all these ignores?
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.
Some of these were already set, and there were already a lot of violation for flake8 so for the most common ones I added it to ignore. If I remove them then I would have to do a lot of editing to code which I am not too sure if you would like it.
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.
@kvesteri could you please confirm if these ignores are ok? I can then submit my PR. Thanks
- new doc dependencies: `mkdocs`, `mkdocs-material` and `mkdocstrings` - auto reference documentation using `mkdocstrings` - moves `pytest` to "tests" dependency group - install only pytest for testing in github-ci - adds pre-commit-hooks **Related Items** *Issues* - Closes python-validators#169 *PRs* - Closes python-validators#173
Unfortunately, pre-commit hooks does not support python3.4, so I had to drop support for python3.4. Its upto @kvesteri to make a decision. closes #169