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

Implement --diff-range-notation option #108

Merged
merged 3 commits into from
Jun 4, 2019
Merged

Implement --diff-range-notation option #108

merged 3 commits into from
Jun 4, 2019

Conversation

nicoddemus
Copy link
Contributor

@nicoddemus nicoddemus commented May 27, 2019

I've also used self.addCleanup to undo setup changes in a few places, after noticing that the patches in test_git_diff.py were not being undone.

addCleanup is considered more resilient and safer because tearDown methods are not executed in case the setUp method fails, which might leave resources around.

They also bring resource creation and deletion closer together, facilitating refactorings.

Fix #105

@coveralls
Copy link

coveralls commented May 27, 2019

Coverage Status

Coverage increased (+0.01%) to 99.15% when pulling 06e0f66 on nicoddemus:diff-opt into 1fafebc on Bachmann1234:master.

@Bachmann1234
Copy link
Owner

Looks good to me. I left a question and curious on your thinking. But if you feel strongly about leaving it as is im happy to leave it and merge

addCleanup is considered more resilient and safer because
tearDown methods are not executed in case the setUp method fails.

They also bring resource creation and deletion closer together,
facilitating refactorings.
@nicoddemus
Copy link
Contributor Author

After you are happy with this changes and this gets merged, could we have another release? I'm happy to open the PR preparing the release if that helps. I don't see instructions on how to make a release, but from what I can gather:

  • Update version in __init__.py
  • Update CHANGELOG
  • Open a PR; after the PR gets approved, someone with write access should push a tag to the PR's HEAD and then finally merge it.

@Bachmann1234
Copy link
Owner

Woot! Thanks for all the work on this. Ill handle getting the release out. let me look at one other PR and get this done tonight

@Bachmann1234 Bachmann1234 merged commit d7793f1 into Bachmann1234:master Jun 4, 2019
@nicoddemus nicoddemus deleted the diff-opt branch June 4, 2019 01:51
@nicoddemus
Copy link
Contributor Author

Awesome, thanks!

@Bachmann1234
Copy link
Owner

just crossing my fingers hoping my recently added 2fa did not break the travis CD pipeline....

@Bachmann1234
Copy link
Owner

woot! https://pypi.org/project/diff-cover/2.1.0/

@nicoddemus
Copy link
Contributor Author

Thanks!

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.

Wondering why diff-cover uses three dot notation for the diff
3 participants