search

spacetelescope / style-guides

An opinionated guide on how we work.
Creative Commons Attribution 4.0 International
55 stars 33 forks source link

Added in issue and pr example tempates #80

Closed brechmos-stsci closed 5 years ago

brechmos-stsci commented 5 years ago

I added in an example issue and PR template that could be added into a github repo. The PR template, in particular, follows the https://github.com/spacetelescope/style-guides/blob/master/guides/git-pr-review.md.

pllim commented 5 years ago

Just curious, did you generate the initial version with the "choose your own adventure" site that you shared?

brechmos-stsci commented 5 years ago

@hcferguson Any comments?
@drdavella You good with my changes?

brechmos-stsci commented 5 years ago

@stscicrawford feel free to comment too :) I would like to start incorporating this into some repos when this is approved.

hcferguson commented 5 years ago

Looks fine. Not really a checklist, but that's okay. Do you want to make recommendations in the template about the scope of the PR -- that it is best to do small, focused PRs rather than collect a bunch of disparate changes into one big PR?

brechmos-stsci commented 5 years ago

@hcferguson I think the focus of the PR (ie, small) should be a discussion in the style guide as opposed to a point in the PR template. At the point that they have opened a PR, the work is probably already done and might be hard to backtrack. (But, if you feel strongly about this, or others do, I can add in a comment like that.)

stscicrawford commented 5 years ago

Small comments on the PR template.

My feeling is that the issue one could be more focused. I think it would be confusing to someone that would be new and I would just have it focus on bug reports with the most important information first (what happened, traceback, and environment/versions)

I'm not sure if this is necessarily better, but this is the one we used in ccdproc as an example (this would need to be modified for STScI):

This is the template for bug reports, if you have a feature request or question you can safely ignore and delete this prefilled text.

Include a description of the problem: What are you trying to do (include your code and the full traceback)? What did you expect?

Include a minimal example to reproduce the issue including output and traceback. The triple backticks make github render this as a multi-line code block.

Don't forget to include the version of astropy, ccdproc and numpy, just copy this into your Python interpreter (without the backticks):

import astropy
print(astropy.__version__)
import ccdproc
print(ccdproc.__version__)
import numpy
print(numpy.__version__)
brechmos-stsci commented 5 years ago

@stscicrawford I hear what you are saying but I think it would be helpful to prompt them for features too. I think there are three ways this could be formatted: 1) aim for bug reports, primarily, as you suggested 2) reformat it a bit so it is similar to below where each section has the "if bug" / "if feature" type of thing, or 3) reformat so the "if bug" / "If feature" wraps the headings, text and prompts.

I am willing to go either way. I kind of like the reformatted below, but will reformat to a more focused bug report if you want.

## Describe the bug / feature

  <!--- If describing a bug:                                                          -->
  <!----   - tell us what happened instead of the expected behavior                   -->       
  <!---    - include the steps to reproduce the bug                                   -->       
  <!---    - if a Traceback is available, include the complete Traceback in the Issue -->       

  <!--- If a change/improvement/feature                                               -->
  <!---    - explain the idea and/or the difference from current behavior             -->

## Possible Solution

  <!--- Not obligatory, but suggest a fix/reason for the bug,                         -->       
  <!--- or ideas how to implement the addition or change (other code, article etc)    -->       

## Context

  <!--- Providing context helps us come up with a solution that is most useful in     -->
  <!--- the real world.                                                               -->       

  <!--- If a bug:                                                                     -->
  <!---    - What are you trying to accomplish?                                       -->       
  <!---    - If it is data specific, please provide any relevant information about    -->
  <!---      the data, or a link to an example, if possible.                          -->       

  <!--- If a new feature:                                                             -->       
  <!---    - How will this feature benefit the field?                                 -->       

## Your Environment

<!--- If a bug:                                                                       -->
<!---     include as many relevant details about the environment                      -->

  * Package Version used:
  * Operating System and version:
  * Include output of conda list:
stscicrawford commented 5 years ago

If you move the environment information into the first section under 'if a bug', then I'd be fine with that. That's an important bit of information so I think it would be good to include it in the first section so it isn't skipped or missed.

brechmos-stsci commented 5 years ago

@stscicrawford I moved the env info to the top part. I reworded a bit too.

hcferguson commented 5 years ago

@brechmos-stsci It's fine to put the comments about scope in a style guide rather than the template. Relevant checklist items might be:

I think right now there is only a guide for reviewers, and I might not think to look there before submitting a PR. If you want to put all the info for submitters and reviewers in one guide, them maybe rename that "PR submitter and reviewer style guide?"

brechmos-stsci commented 5 years ago

@stscicrawford I saw you reference changes, just saw them now. Will do...

brechmos-stsci commented 5 years ago

@hcferguson I changed the text to the "fixes" part to say "all relevant issues" rather than just "the issue" to hopefully tweak people to put all the relevant ones. The PR reviewer guide is specific to people reviewing PRs rather than writing PRs. There was some discussion that a "write a PR" style guide might be helpful but I think that is separate from "reviewing a PR" (not to say that reading the "reviewing a PR" might be helpful anyway!