Skip to content
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

Add code style and github issue guides #463

Merged
merged 6 commits into from Mar 16, 2020
Merged

Add code style and github issue guides #463

merged 6 commits into from Mar 16, 2020

Conversation

dsherry
Copy link
Collaborator

@dsherry dsherry commented Mar 9, 2020

No description provided.

@dsherry dsherry added the documentation Improvements or additions to documentation label Mar 9, 2020
@dsherry dsherry self-assigned this Mar 9, 2020
@codecov
Copy link

codecov bot commented Mar 9, 2020

Codecov Report

Merging #463 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #463   +/-   ##
=======================================
  Coverage   98.42%   98.42%           
=======================================
  Files         104      104           
  Lines        3427     3427           
=======================================
  Hits         3373     3373           
  Misses         54       54           

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 67c2a52...9ec8f8e. Read the comment docs.

contributing.md Show resolved Hide resolved
jeremyliweishih
jeremyliweishih previously requested changes Mar 9, 2020
contributing.md Show resolved Hide resolved
contributing.md Show resolved Hide resolved
contributing.md Outdated
* Make the title as short and descriptive as possible.
* Make sure the body is concise and gets to the point quickly.
* Check for duplicates before filing.
* For bugs, a good general outline is: problem summary, root cause if known, symptoms and scope, proposed solution(s), and next steps.
Copy link
Contributor

@angela97lin angela97lin Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also be useful to provide minimal steps to reproduce bug, if possible?

Copy link
Collaborator Author

@dsherry dsherry Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good point!

contributing.md Show resolved Hide resolved
contributing.md Outdated
* Keep things simple. Any complexity must be justified in order to pass code review.
* Be aware that while we love fancy python magic, there's usually a simpler solution which is easier to understand!
* Make PRs as small as possible! Consider breaking your large changes into separate PRs. This will make code review easier, quicker, less bug-prone and more effective.
* In the name of every branch you create, include your initials and the associated issue number if applicable.
Copy link
Contributor

@angela97lin angela97lin Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular format we want to enforce for this? I know you usually end up doing initials_pr#_moredescription or something along those lines?

Copy link
Collaborator Author

@dsherry dsherry Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, on further reflection, I think the branch should include an issue number when relevant, but other than that, I'm down with whatever, don't have a strong opinion haha. Yes, personally, I like to do <initials>_<issue number>_<short description>.

Ok if I update this to say "In the name of every branch you create, include the associated issue number if applicable" ?

Copy link
Contributor

@angela97lin angela97lin Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, sounds good :)

@dsherry dsherry changed the title Add code style and github issue guides [WIP] Add code style and github issue guides Mar 9, 2020
@angela97lin
Copy link
Contributor

angela97lin commented Mar 9, 2020

This could be a tiny thing but what do you think about choosing a consistent tense for changelog items? Just going through the changelog right now for the release, and thought it'd be nice to have some consistency there!

@dsherry dsherry changed the title [WIP] Add code style and github issue guides Add code style and github issue guides Mar 13, 2020
@dsherry dsherry requested a review from jeremyliweishih Mar 13, 2020
contributing.md Outdated
* If your changes alter the following please fix them as well:
* Docstrings - if your changes render docstrings invalid
* API changes - if you change the API update `docs/source/api_reference.rst`
* Documentation - run the documentation notebooks locally to ensure everything is logical and works as intended

* Update the "Future Release" section of the changelog (`docs/source/changelog.rst`) to include your pull request. Add a description of your PR to the subsection that most closely matches your contribution:
* Update the "Future Release" section of the changelog (`docs/source/changelog.rst`) to include an entry for your pull request. Write your entry in past tense, i.e. "added fizzbuzz impl." Add a description of your PR to the subsection that most closely matches your contribution:
Copy link
Collaborator Author

@dsherry dsherry Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

@dsherry dsherry Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this seem ok?

Copy link
Contributor

@angela97lin angela97lin Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, looks great :D

@dsherry dsherry requested review from angela97lin and kmax12 Mar 13, 2020
kmax12
kmax12 approved these changes Mar 13, 2020
Copy link
Contributor

@kmax12 kmax12 left a comment

looks good. only one comment on something to add

contributing.md Outdated
* If your changes alter the following please fix them as well:
* Docstrings - if your changes render docstrings invalid
* API changes - if you change the API update `docs/source/api_reference.rst`
* Documentation - run the documentation notebooks locally to ensure everything is logical and works as intended

* Update the "Future Release" section of the changelog (`docs/source/changelog.rst`) to include your pull request. Add a description of your PR to the subsection that most closely matches your contribution:
* Update the "Future Release" section of the changelog (`docs/source/changelog.rst`) to include an entry for your pull request. Write your entry in past tense, i.e. "added fizzbuzz impl." Add a description of your PR to the subsection that most closely matches your contribution:
Copy link
Contributor

@kmax12 kmax12 Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we added info about how/when they should document breaking changes in the changelog?

Copy link
Collaborator Author

@dsherry dsherry Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

kmax12
kmax12 approved these changes Mar 16, 2020
Copy link
Contributor

@angela97lin angela97lin left a comment

🚢

@dsherry dsherry dismissed jeremyliweishih’s stale review Mar 16, 2020

Updated PR to address comments :)

@dsherry dsherry merged commit 44142d6 into master Mar 16, 2020
2 checks passed
@dsherry dsherry deleted the ds_code_style branch Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants