-
-
Notifications
You must be signed in to change notification settings - Fork 420
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
fix contributing guidelines #506
fix contributing guidelines #506
Conversation
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.
Looks good to me! I'll leave this for a little if @jni wants to comment, then I'll merge
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.
@kne42 I'd revert the removal of the black config explanation, as I think it's relevant.
@@ -39,7 +39,6 @@ pre-commit install | |||
|
|||
With help of pre-commit, your future commits to this project will be reformatted with our black configuration, | |||
which includes settings `skip-string-normalization = true` and `max-line-length = 79`. | |||
On the command-line, our configuration would be equivalent to `black -S -l 79 -t py36 --exclude vendored\|_vispy .`. |
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 remove this? I think it's very useful for other libraries considering adding black. (such a thing was proposed at scikit-image a few weeks ago)
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 removed it because we have to update it every time we change our black
config and right now our config is super long (we exclude like 10 different things)
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.
How about I add a reference to the file where these settings are stored instead?
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.
Besides, it's not our job to document black; if people are interested in using it, they should look at the official documentation instead.
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.
adding a reference to the file where this info is stored sounds like a reasonable compromise to me, especially as the exclude
list might continue to change
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.
Great, I'm into the reference idea.
Codecov Report
@@ Coverage Diff @@
## master #506 +/- ##
=======================================
Coverage 78.36% 78.36%
=======================================
Files 118 118
Lines 12098 12098
=======================================
Hits 9480 9480
Misses 2618 2618 Continue to review full report at Codecov.
|
fix errors in contributing guidelines
closes #357