Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ENH: Allow not printing coverage report #830

Merged
merged 3 commits into from
Jan 27, 2016

Conversation

larsoner
Copy link
Contributor

@larsoner larsoner commented Aug 3, 2014

Code changes proposed to address #829.

Closes #829.

@@ -29,6 +29,7 @@ class Coverage(Plugin):
coverInstance = None
coverErase = False
coverMinPercentage = None
coverNoPrint = False
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd flip the sense of this and set coverPrint = True. Then...

@larsoner
Copy link
Contributor Author

Agreed, double-negatives are not good. Comments addressed.

@larsoner
Copy link
Contributor Author

Latest commit adds a simple test (just making sure the switch can be used).

@hello-josh
Copy link

What is the status of this PR? I would like this option as well.

@larsoner
Copy link
Contributor Author

Ready to go from my end as of ~6 months ago.

@cp2587
Copy link

cp2587 commented Apr 21, 2015

Can this be merge ? Would be a good addition

@larsoner
Copy link
Contributor Author

It can no longer be automatically merged (merge conflict). I'm happy to rebase, though, if the maintainers want this addition.

@larsoner
Copy link
Contributor Author

@jszakmeister any comment on this one? I'm happy to rebase if necessary. Would be nice to have this option eventually.

@larsoner
Copy link
Contributor Author

Rebase complete and Travis is happy.

@jszakmeister
Copy link
Contributor

A couple of comments. I've seen others do this, but I'm not real fond of the "FIX", "STY", and "ENH" prefixes on commit messages. Also, there were a number of unrelated stylistic fixes in there too. I realize your editor probably helped you "fix" them, but it detracts from the actual changes that you make. I mention it because a number of projects would outright reject patches that did that, and I think it's best to error on the side of caution and refrain from it when contributing to other projects.

That said, thank you. I'll merge this now.

FWIW, I'm stepping down as maintainer, so Nose will be unmaintained after I cut the next patch release.

jszakmeister added a commit that referenced this pull request Jan 27, 2016
ENH: Allow not printing coverage report
@jszakmeister jszakmeister merged commit 6e11bf9 into nose-devs:master Jan 27, 2016
@larsoner larsoner deleted the no-print branch January 27, 2016 22:11
@larsoner
Copy link
Contributor Author

Thanks for the comments, (re-)review, merge, and maintenance effort (even though it's coming to an end soon).

FWIW I do the prefixes because to my eye they have become a de facto standard in the scientific Python ecosystem (e.g., numpy, scipy, scikit-learn), but perhaps they're less common elsewhere in Python.

@jszakmeister
Copy link
Contributor

Thanks for the comments, (re-)review, merge, and maintenance effort (even though it's coming to an end soon).

Thank you.

FWIW I do the prefixes because to my eye they have become a de facto standard in the scientific Python ecosystem (e.g., numpy, scipy, scikit-learn), but perhaps they're less common elsewhere in Python.

As someone who has contributed to a number of open source projects for the past 15+ years, it's a good idea to see what the project does and follow their standards, instead of having them accept your own. It's makes the processes smoother, and it's one less thing that a maintainer has to comment on and tell you to fix. :-)

Thanks again. :-)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: Allow not printing coverage report
4 participants