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

Fix master failure on travis (relating to ASM raise-assertion). #524

Closed
wants to merge 1 commit into from

Conversation

extemporalgenome
Copy link

Checklist

  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the master branch.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added in line documentation to the code I modified

Short description of what this PR does:

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Oct 31, 2017
@codecov
Copy link

codecov bot commented Oct 31, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@200be7c). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #524   +/-   ##
=========================================
  Coverage          ?   83.15%           
=========================================
  Files             ?       30           
  Lines             ?     1027           
  Branches          ?      162           
=========================================
  Hits              ?      854           
  Misses            ?       83           
  Partials          ?       90
Impacted Files Coverage Δ
sendgrid/helpers/mail/asm.py 80% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 200be7c...3412260. Read the comment docs.

@codecov
Copy link

codecov bot commented Oct 31, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@200be7c). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #524   +/-   ##
=========================================
  Coverage          ?   83.15%           
=========================================
  Files             ?       30           
  Lines             ?     1027           
  Branches          ?      162           
=========================================
  Hits              ?      854           
  Misses            ?       83           
  Partials          ?       90
Impacted Files Coverage Δ
sendgrid/helpers/mail/asm.py 80% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 200be7c...3412260. Read the comment docs.

@@ -16,7 +16,7 @@ def __init__(self, group_id=None, groups_to_display=None):
self._group_id = group_id
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually came across the one you fixed, this one and one other one too: #484 (review)

# Shouldn't this...
self._group_id = group_id

# be this?
self.group_id = group_id

Maybe you could fix this one and add another commit for the one inside sendgrid/helpers/mail/category.Category as well?

# Shouldn't this...
self._name = name

# be this?
self.name = name

Copy link
Author

Choose a reason for hiding this comment

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

My goal here was to fix tests on the master branch -- we should probably address the wider question of dog-fooding setters inside init (I agree with you) after master gets green.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lettuce means that the master failure was caused by a typo, of which two others exist without unit tests to fail. It might be nice to fix them in this PR since they're the "same" problem.

@gabrielkrell
Copy link
Contributor

I'm both pleased that adding the ASM validation stuff exposed an existing bug and aggravated that I didn't run tests locally and catch it myself.

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Nov 1, 2017
@thinkingserious thinkingserious added type: community enhancement feature request not on Twilio's roadmap difficulty: medium fix is medium in difficulty status: waiting for feedback waiting for feedback from the submitter labels Feb 27, 2018
@thinkingserious thinkingserious deleted the fix-master-asm-failure branch September 11, 2019 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium fix is medium in difficulty status: code review request requesting a community code review or review from Twilio status: waiting for feedback waiting for feedback from the submitter type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
5 participants