Skip to content
This repository was archived by the owner on Jul 19, 2025. It is now read-only.

Remove rollout call for quality stats #70

Merged
merged 2 commits into from
Apr 20, 2015
Merged

Conversation

gdiggs
Copy link
Contributor

@gdiggs gdiggs commented Apr 20, 2015

Now that this feature is enabled for all repos, we can remove the check

@gdiggs
Copy link
Contributor Author

gdiggs commented Apr 20, 2015

/cc @codeclimate/review

@@ -13,7 +13,7 @@ def initialize(payload, repo_config)
end

def success_message
if @repo_config.pr_status_quality_stats?
if issue_counts_in_payload?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this conditional still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way these issues are passed and particularly how they are set in this class means they could be nil. I figured it was worth it to keep this check so that if the counts aren't in the request, we can still report statuses on github PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

That might be vestigial. Are you aware of any cases where it makes sense that they'd be nil?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. If we are the one controlling the message, I think this conditional is unnecessary. YAGNI.

@noahd1
Copy link
Contributor

noahd1 commented Apr 20, 2015

👍

gdiggs added a commit that referenced this pull request Apr 20, 2015
@gdiggs gdiggs merged commit 55828fb into master Apr 20, 2015
@gdiggs gdiggs deleted the gd-remove-status-rollout branch April 20, 2015 20:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants