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 badges, minimal codecov, mk2 #352

Merged
merged 3 commits into from
Mar 27, 2021
Merged

Conversation

bollwyvl
Copy link
Collaborator

@bollwyvl bollwyvl commented Mar 26, 2021

@bollwyvl
Copy link
Collaborator Author

Oooh, i do see a check already (presumably because 99ba18e was already reported on).

@bollwyvl
Copy link
Collaborator Author

Cool. So this PR now shows all the bells and whistles. We can turn them down by customizing a codecov.yml but the defaults seem fine and shouty at me for having broken coverage.

The goal on this isn't to make anybody bad about their contributions, but rather to help reduce any back-and-forth of hey, it'd be nice if we had a test for this new whizzbang feature.

@jorisvandenbossche
Copy link
Member

Personally, I would maybe prefer to only have the codecov status check, and not the comment (my experience with codecov from other projects is that the commenting can be quite noisy ..)

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Mar 26, 2021 via email

@drammock
Copy link
Collaborator

FYI, in the "files changed" view of PRs, you can show/hide codecov's (and everyone else's) comments by pressing "a". My personal experience (with codecov in MNE-Python repo) is that the inline comments can be helpful if there's just one or two of them, but more than that and they quickly become a nuisance. Learning the "a" shortcut allowed us to keep inline comments turned on.

@choldgraf
Copy link
Collaborator

Why would this PR trigger any red in the codecov though? Isn't it just CI/CD changes?

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Mar 27, 2021

Isn't it just CI/CD changes?

I added the --cov-branch switch.

update: took it out again so it should come up ✔️

@pydata pydata deleted a comment from codecov bot Mar 27, 2021
@bollwyvl
Copy link
Collaborator Author

Ha, i deleted the comment, and it put it back. But have pushed with a minimal codecov. Guess if i delete it again (after it reports) it shouldn't put it back.

us to keep inline comments turned on.

I do find the annotations useful, but we'll see how it plays out. a is a good hotkey to know, for sure! I'd love to figure out a way to tie that all the way back in #294, but that sounds ... hard.

@bollwyvl
Copy link
Collaborator Author

Ok, it reported, threw the threshold flag. Will try re-deleting the comment, maybe re-kicking...

@pydata pydata deleted a comment from codecov bot Mar 27, 2021
@bollwyvl bollwyvl changed the title add badges, branch coverage, mk2 add badges, minimal codecov, mk2 Mar 27, 2021
@bollwyvl
Copy link
Collaborator Author

I do think it would be good to put the --cov-branch back... but perhaps on a PR intended to actually improve coverage :)

@choldgraf
Copy link
Collaborator

@bollwyvl wanna open up an issue for "improve test coverage and add the flag bag in when that happens"? I'm assuming this is ready to go otherwise?

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Mar 27, 2021 via email

@choldgraf choldgraf merged commit 9908530 into pydata:master Mar 27, 2021
@choldgraf
Copy link
Collaborator

cool - thanks for adding this!

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.

4 participants