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

Disable codecov comments #1949

Merged
merged 1 commit into from
Oct 28, 2016
Merged

Disable codecov comments #1949

merged 1 commit into from
Oct 28, 2016

Conversation

MorrisJobke
Copy link
Member

Was requested in #1903 (comment)

Signed-off-by: Morris Jobke <[email protected]>
@mention-bot
Copy link

@MorrisJobke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @LukasReschke to be a potential reviewer.

@MorrisJobke
Copy link
Member Author

cc @LukasReschke @jancborchardt

@MorrisJobke
Copy link
Member Author

@MorrisJobke MorrisJobke added this to the Nextcloud 11.0 milestone Oct 28, 2016
@MorrisJobke MorrisJobke added the 3. to review Waiting for reviews label Oct 28, 2016
@eppfel
Copy link
Member

eppfel commented Oct 28, 2016

👍

@ChristophWurst
Copy link
Member

👍 🙏 thanks

@ChristophWurst ChristophWurst merged commit 96c35d3 into master Oct 28, 2016
@ChristophWurst ChristophWurst deleted the disable-codecov-comments branch October 28, 2016 23:05
@LukasReschke
Copy link
Member

LukasReschke commented Oct 29, 2016

Why? I do dislike the removal hugely 👎

@LukasReschke
Copy link
Member

LukasReschke commented Oct 29, 2016

Its an easy way to see which new files have received tests and which don't etc…

And most people here do simply ignore the results in the bar below.

I'm totally against removing this.

@@ -6,6 +6,4 @@ coverage:
round: down
range: "70...100"

comment:
layout: "header, diff, changes, sunburst, uncovered, tree"
Copy link
Member

Choose a reason for hiding this comment

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

At least header should really stay… Removing this is just another easy way to ignore coverage. Awesome! 🚀

Copy link
Member

Choose a reason for hiding this comment

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

(don't care about the other values)

@LukasReschke
Copy link
Member

LukasReschke commented Oct 29, 2016

Why? I do dislike the removal hugely 👎

And "Jan is annoyed by being notified once on each of his pull request" is NOT a valid reason before that comes up. 😉

@eppfel
Copy link
Member

eppfel commented Oct 29, 2016

@LukasReschke I'm open to reconsidered, as I think there is a conflict of interest between developers focus on testing and others that are annoyed of the comments:grin:. But if you want to see changes, it should be the tree or diff view, not the header.
But why not improve the check / commit status. You could be more strict and then you can't ignore it.
Move discussion to an issue?

@rullzer
Copy link
Member

rullzer commented Oct 29, 2016

I'm with @LukasReschke here. It is an easy overview.

@jancborchardt
Copy link
Member

@LukasReschke @rullzer the reason for removing this is because it makes the discussion of a pull request hugely technical, and intimidating for any new contributors.

most people here do simply ignore the results in the bar below.

If you ignore the results in the bar below, then pushing them up everyone’s faces is not going to change that. It will just result in being annoyed.

@jancborchardt
Copy link
Member

It’s like having a bot posting to an IRC channel – misunderstanding the medium. A Github thread is a discussion between humans, not a space for bots to dump their results.

@rullzer
Copy link
Member

rullzer commented Nov 2, 2016

Well if people changed code they should add/change tests. Having a clear overview of what was added etc is a lot easier then telling. Click on the status of codecov. Then see where you need to add coverage.

A github PR is the perfect place for that info. It is where we discuss if a new piece of code should go into the project. I'm sorry but a pull request is technical.

@LukasReschke
Copy link
Member

LukasReschke commented Nov 2, 2016

This is just leading to having coverage ignored… Just had another PR today especially because the status simply got ignored. Jesus. Folks. This is proper software development and not a YOLO here.

Just because some of you are like "OMG I GET NOTIFIED ONCE" we can't just throw away important tools ❗️❗️❗️

@LukasReschke
Copy link
Member

LukasReschke commented Nov 2, 2016

@LukasReschke @rullzer the reason for removing this is because it makes the discussion of a pull request hugely technical, and intimidating for any new contributors.

You gotta be kidding me, right?

A PULL REQUEST IS TECHNICAL. THIS IS ABOUT CODE.

Sorry to say it that clear. But this is bullshit. We do software development and not a "let's keep away any tools that tell you your code is bad."

@LukasReschke
Copy link
Member

Revert in #1982

@LukasReschke
Copy link
Member

LukasReschke commented Nov 2, 2016

If you ignore the results in the bar below, then pushing them up everyone’s faces is not going to change that. It will just result in being annoyed.

The feedback in the result bar is DIFFERENT from what is shown in the list. The feedback in the bar will also be green even if coverage effectively decreased (because it could relatively increase). See #1347 for the green bar which effectively is NOT good if you look at the actual comment by the bot.

That people ignore it: Well. I don't care. At least other people that don't can actually do the right thing and tell people to fix their stuff before merging. I have a very low amount of tolerance for removing tools that assure code quality.

@te-online
Copy link
Contributor

it makes the discussion of a pull request hugely technical

Technical as in not (instantly) human readable.

I think there is nothing wrong with the information itself, but with the way how a (obviously) machine-generated output mixes with a conversation of (hopefully) real people. It doesn't feel right, because it is embedded in the conversation context (and not distinguishable from people's comments), but not really part of that conversation.

One or two sentences, describing the current status in human readable language (and those could of course be machine-generated) and a maybe link for more details would be just enough. But I guess that not a configurable option, so for now I would support the revert as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants