-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Github templates #1954
Github templates #1954
Conversation
8822646
to
c568b01
Compare
Just double check, I think Josh might have already filed something similar
|
Nope! I just raised the issue - it's been very busy for me of late, and I
|
Philosophically, I think we need to take a minimal approach to this. These templates are effectively barriers to contribution or even getting bugs reported, which are both behaviors we want to facilitate rather than have another big checklist. Since this is an important public-facing aspect of our project, I'll be pretty fine grained in my review. |
@@ -0,0 +1,19 @@ | |||
## Description | |||
[What is wrong? What the expected and current behaviours are?] |
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.
[What is wrong? How would you expect this to work?]
How are we supporting new feature requests through this ? |
@vighneshbirodkar Great point. Basically, we have to make it clear nearly all fields are optional using language like "If possible..." |
@vighneshbirodkar See my message in #1949 . I'm in favor of the approach where many of the blocks have double meaning/purpose. |
@JDWarner thank you very much for reflecting on this! I've made all proposed changes, except one. |
Should we also include a section for attaching images ? Comparison of new and old output makes a lot of things clear. |
@vighneshbirodkar |
@soupault Then can we somehow request for images when new features are submitted ? I always try to attach the output from the example script |
@vighneshbirodkar This should do. UPD: I take your point, this should help a lot when people are submitting new features. Usually, nobody cares to pin an example of the result to the PR. |
This LGTM. 👍 |
## Checklist | ||
[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:] | ||
- [ ] [PEP8 compliance](https://www.python.org/dev/peps/pep-0008/) | ||
- [ ] Docstrings for all functions |
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.
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 wish Github had internal URL-shortener...
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.
You can link to it within the document and put the full url at the bottom
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.
We could, but yikes - perusing the actual standard is equivalent to reading a small book. Intimidating! Perhaps directing them to imitate existing docstrings would be more friendly? "Docstrings for functions, with style consistent across the package" or something similar?
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 view this more as a getting-started pointer, rather than sending people out with homework ;)
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.
@JDWarner I think the style should be actually aligned with numpydoc
, not with random skimage
example (which can become out-of-date one day).
I agree that the standard is TLDR 😃 , but (at the same time) is a fairly good document.
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.
All well and true, but it's our responsibility as maintainers to ensure docstring style is up to date and consistent.
It's a good document, but it's just far and away more than any new contributor needs. If a docstring is subtly incorrect or needs a new feature, again, it's our responsibility to point them in the right direction in the course of the PR.
I do strongly feel we don't want to front load this. I was on the fence about including any links at all.
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 understand.
Probably the best solution will be to keep the template as is for a while so to see which fields are actually used and which do cause obstacles.
Regarding the docsting guide: it should be better to address the detailed guide in CONTRIBUTING.txt
.
@JDWarner, how do you feel about us adding something like |
blink1073 👍, that would be worth investigating. Probably in a separate PR, though. |
f003661
to
f6f5d5a
Compare
What if we linked to this, which demonstrates by example and is the canonical implementation: https://github.com/numpy/numpy/blob/master/doc/example.py |
|
||
## Checklist | ||
[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:] | ||
- [ ] Style in [compliance with PEP8](https://www.python.org/dev/peps/pep-0008/) |
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.
Changing my own mind here, but "Clean style in the spirit of PEP8" might be even better. Mainly considering PEP8 tells us to break it when we feel justified, and it's friendlier than 'compliance'.
@@ -0,0 +1,22 @@ | |||
## Description | |||
[What is wrong? How would you expect this to work?] |
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 would reformulate, maybe "Describe your problem, feature proposal, etc."
@ahojnnes How do you feel about the updated version? |
[Please provide general introduction to the issue/proposal.] | ||
|
||
|
||
## Reproduction |
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.
"Reproduction" sounds slightly misplaced here, but I'll let native speakers comment :-)
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's not much off, but maybe 'Reproducing the issue' would be better.
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.
or "Attachements"
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.
"Steps to reproduce"?
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.
Fine with @stefanv 's version.
In general, I think, we should keep this as short as possible. Until now, I am not sure if we really had problems with too many incomplete issue reports? |
@ahojnnes I suppose the goal is not just to integrate the new technique, but to exploit it to achieve some particular improvements. |
All things are done. 👻 |
@@ -0,0 +1,16 @@ | |||
## Description | |||
[Please provide general introduction to the issue/proposal.] |
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.
provide a general
I think we're converging. Thanks for your patience with our bike shedding! |
@stefanv I hope that the equilibrium has at least couple of words in the templates 😉 . |
@soupault If I had my way, very few ;) |
One more iteration here 🔨 . |
That looks fine to me. Thanks for your patience @soupault ! |
@JDWarner @blink1073 @ahojnnes Any opinions, gentlemen? |
Great, thanks @soupault! |
Addresses #1949 .
Added templates, moved contributing guidelines from the root.