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

added pull request template and docs issue template #71

Merged
merged 4 commits into from Jun 14, 2018

Conversation

2 participants
@leifwalsh
Copy link
Contributor

leifwalsh commented Jun 3, 2018

Closes #47
Closes #52

@leifwalsh leifwalsh self-assigned this Jun 3, 2018

@leifwalsh leifwalsh added this to To do in General improvements via automation Jun 3, 2018

@leifwalsh leifwalsh requested a review from thejunglejane Jun 3, 2018

- [ ] Added a note to the [changelog](https://github.com/twosigma/marbles/blob/master/docs/changelog.rst)?

#### Open Questions and Pre-Merge TODOs
- [ ] Use github checklists. When solved, check the box and explain the answer.

This comment has been minimized.

@thejunglejane

thejunglejane Jun 8, 2018

Contributor

s/github/GitHub

_What new tests were added to cover new features or behaviors?_

## Checklist
_Make sure your change addresses the following concerns, if applicable._

This comment has been minimized.

@thejunglejane

thejunglejane Jun 8, 2018

Contributor

"Make sure you did the following (if applicable):"

## Tests for New Behavior
_What new tests were added to cover new features or behaviors?_

## Checklist

This comment has been minimized.

@thejunglejane

thejunglejane Jun 8, 2018

Contributor

Can we make this checklist imperative?

  • Signed and returned a copy of the CLA
  • Added tests for any new features or behaviors
  • Ran tox -e flake8 to make sure code style is consistent
  • Built and reviewed the docs
  • Added a note to the changelog
_Make sure your change addresses the following concerns, if applicable._
- [ ] Did you add tests for any new features or behaviors?
- [ ] Did you run ``tox -e flake8`` to make sure code style is consistent?
- [ ] Did you build and review the docs?

This comment has been minimized.

@thejunglejane

thejunglejane Jun 8, 2018

Contributor

What you and I often do is throw up a link where the reviewer can see the built docs. Can we encourage people to do this (especially if doc changes are significant), or do you think that will be too cumbersome?

This comment has been minimized.

@leifwalsh

leifwalsh Jun 9, 2018

Author Contributor

I don't think that would be easy to do for someone on a different network, they'd have to open ports and it would be weird. I think we can just clone the branch locally and build to review, but maybe we can some day teach Travis to upload rendered docs from the PR build somewhere.

## Checklist
_Make sure your change addresses the following concerns, if applicable._
- [ ] Did you add tests for any new features or behaviors?
- [ ] Did you run ``tox -e flake8`` to make sure code style is consistent?

This comment has been minimized.

@thejunglejane

thejunglejane Jun 8, 2018

Contributor

Could we/do we want to add a flake8 check to the PR?

This comment has been minimized.

@leifwalsh

leifwalsh Jun 9, 2018

Author Contributor

The travis build should fail if flake8 fails already, and this would mark the PR status.

@leifwalsh leifwalsh moved this from To do to In progress in General improvements Jun 11, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jun 11, 2018

Codecov Report

Merging #71 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #71   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines         739    739           
  Branches      141    141           
=====================================
  Hits          739    739

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80cee93...751ea29. Read the comment docs.

@leifwalsh leifwalsh moved this from In progress to Reviewed in General improvements Jun 14, 2018

@leifwalsh leifwalsh merged commit 932993b into master Jun 14, 2018

3 checks passed

codecov/patch Coverage not affected when comparing ddfabcb...751ea29
Details
codecov/project 100% remains the same compared to ddfabcb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

General improvements automation moved this from Reviewed to Done Jun 14, 2018

@leifwalsh leifwalsh deleted the templates branch Jun 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment