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 params type checking for mail helper methods #720

Closed
wants to merge 3 commits into from

Conversation

f1x3d
Copy link

@f1x3d f1x3d commented Oct 29, 2018

Resolves #701

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:

  • Add the decorator for type checking of method parameters for mail helper
  • Add a test that verifies the implemented functionality

Since the test coverage was not a part of the issue's acceptance criteria, I've included only one basic test.
Please let me know if you want me to add more tests to cover all methods from the mail helper, which are being type checked now.

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Oct 29, 2018
@SendGridDX
Copy link

SendGridDX commented Oct 29, 2018

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Oct 29, 2018

Codecov Report

Merging #720 into master will increase coverage by 0.24%.
The diff coverage is 91.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #720      +/-   ##
==========================================
+ Coverage   85.03%   85.27%   +0.24%     
==========================================
  Files          35       36       +1     
  Lines        1156     1216      +60     
  Branches      172      179       +7     
==========================================
+ Hits          983     1037      +54     
- Misses         90       94       +4     
- Partials       83       85       +2
Impacted Files Coverage Δ
sendgrid/helpers/mail/subscription_tracking.py 88.57% <100%> (+0.69%) ⬆️
sendgrid/helpers/mail/__init__.py 100% <100%> (ø) ⬆️
sendgrid/helpers/mail/bcc_settings.py 91.3% <100%> (+1.3%) ⬆️
sendgrid/helpers/mail/bypass_list_management.py 92.85% <100%> (+1.19%) ⬆️
sendgrid/helpers/mail/mail.py 93.82% <100%> (+0.11%) ⬆️
sendgrid/helpers/mail/asm.py 85.18% <100%> (+1.18%) ⬆️
sendgrid/helpers/mail/spam_check.py 96.42% <100%> (+0.27%) ⬆️
sendgrid/helpers/mail/email.py 97.43% <100%> (+0.13%) ⬆️
sendgrid/helpers/mail/header.py 86.36% <100%> (+0.64%) ⬆️
sendgrid/helpers/mail/click_tracking.py 90.47% <100%> (+1%) ⬆️
... and 10 more

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 cedb4cc...8636545. Read the comment docs.

@thinkingserious
Copy link
Contributor

Hello @f1x3d,

Thanks again for the PR!

It's HACKTOBERFEST! We want to show our appreciation by sending you some special Hacktoberfest swag. If you have not already, could you please fill out this form so we can send it to you? Thanks!

Team SendGrid DX

@thinkingserious
Copy link
Contributor

Hello @f1x3d,

Thanks again for the PR!

It's HACKTOBERFEST! We want to show our appreciation by sending you some special Hacktoberfest swag. If you have not already, could you please fill out this form so we can send it to you? Thanks!

Team SendGrid DX

@f1x3d
Copy link
Author

f1x3d commented Nov 2, 2018

Hello @thinkingserious,

I've filled the form.

Thank you so much for joining the Hacktoberfest event and rewarding contributors with some awesome swag!

@thinkingserious thinkingserious changed the base branch from master to main July 28, 2020 14:35
@@ -19,6 +19,7 @@ install:
- pip install pypandoc
- pip install coverage
- pip install codecov
- pip install wrapt
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add a new dependency instead of using built-in decorators?

if expected_type is str:
expected_type = six.string_types

if not isinstance(arg_val, expected_type):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worried that this is going to cause more breaking changes than is functionally necessary. For example, with this PR adding strict type checking broke a functional implementation.

@childish-sambino
Copy link
Contributor

Closing until PR comments have been addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: hard fix is hard in difficulty status: code review request requesting a community code review or review from Twilio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type Exception Handling for the Mail Helper
5 participants