Skip to content

Refactoring doctests #5518

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

Merged
merged 1 commit into from
Jun 28, 2019
Merged

Refactoring doctests #5518

merged 1 commit into from
Jun 28, 2019

Conversation

AmirElkess
Copy link
Contributor

head-fork: AmirElkess/pytest
compare: refactor

base-fork: pytest-dev/pytest
base: master

@codecov
Copy link

codecov bot commented Jun 28, 2019

Codecov Report

Merging #5518 into master will increase coverage by <.01%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5518      +/-   ##
=========================================
+ Coverage   96.09%   96.1%   +<.01%     
=========================================
  Files         117     117              
  Lines       25647   25645       -2     
  Branches     2483    2482       -1     
=========================================
  Hits        24646   24646              
+ Misses        696     695       -1     
+ Partials      305     304       -1
Impacted Files Coverage Δ
src/_pytest/doctest.py 96.62% <91.66%> (+0.71%) ⬆️

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 3c9b46f...bf39e89. Read the comment docs.

@nicoddemus
Copy link
Member

Hi @AmirElkess,

Thanks a lot for taking this issue and submitting a PR, we really appreciate it. Congratulations on your first open source contribution. 👍

Regarding the code itself, your PR is fine, thanks!

About title and description I have a few tips: it helps to use a short title and small description so we maintainers can see what the PR is about at a glance. Including the issue number that the PR is related to is also good (#5513 in this example), as GitHub will automatically create links between issues and PR.

Another tip is to use a keyword to close issues automatically when merging, in this case it would be Fix #5513 (or any of the other keywords known by GitHub).

Having said all the above, here is how I would have submitted this PR:

title: Remove fallback for legacy inspect.unwrap

description: Fix #5513

Note that I wrote a very short description, that's because the issue explains the problem well and the PR is very straightforward, so I just used the "Fix #" magic comment so GitHub will close the related issue automatically when the PR is merged. 👍


About the changelog entry, we usually add one, but given that this is an internal change which doesn't affect users in any way, we don't really need a CHANGELOG for that, so none is necessary! 👍

I'm merging this as is then. Thanks again!

@nicoddemus nicoddemus merged commit e6ffa78 into pytest-dev:master Jun 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants