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

PEP8 Fixes and String Formatting Enhancement #697

Merged
merged 2 commits into from
Oct 30, 2018

Conversation

vkmrishad
Copy link
Contributor

@vkmrishad vkmrishad commented Oct 20, 2018

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 inline documentation to the code I modified

Short description of what this PR does:

  • PEP 8 Fixes.
  • String Formatting Enhancement.

If you have questions, please send an email to SendGrid, or file a GitHub Issue in this repository.

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

codecov bot commented Oct 20, 2018

Codecov Report

Merging #697 into master will increase coverage by 0.27%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #697      +/-   ##
==========================================
+ Coverage   84.77%   85.04%   +0.27%     
==========================================
  Files          35       35              
  Lines        1156     1157       +1     
  Branches      172      172              
==========================================
+ Hits          980      984       +4     
  Misses         90       90              
+ Partials       86       83       -3
Impacted Files Coverage Δ
sendgrid/helpers/inbound/send.py 93.75% <ø> (ø) ⬆️
sendgrid/helpers/mail/validators.py 44.44% <ø> (ø) ⬆️
sendgrid/helpers/mail/content.py 84.61% <ø> (ø) ⬆️
sendgrid/helpers/mail/exceptions.py 66.66% <ø> (ø) ⬆️
sendgrid/helpers/inbound/config.py 97.05% <100%> (+3.11%) ⬆️
sendgrid/helpers/mail/spam_check.py 96.15% <0%> (+7.69%) ⬆️

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 0b68316...4762039. Read the comment docs.

@vkmrishad
Copy link
Contributor Author

hi @misterdorm,
Can you please check this PR

@misterdorm
Copy link
Contributor

Replaces #673

@misterdorm misterdorm added status: work in progress Twilio or the community is in the process of implementing hacktoberfest difficulty: medium fix is medium in difficulty type: twilio enhancement feature request on Twilio's roadmap and removed status: code review request requesting a community code review or review from Twilio labels Oct 22, 2018
@vkmrishad
Copy link
Contributor Author

@misterdorm,
This will replace #673

@vkmrishad
Copy link
Contributor Author

@misterdorm
Please merge this PR

@vkmrishad
Copy link
Contributor Author

@42B @misterdorm
Can you please merge this PR.

@@ -144,8 +144,11 @@ def test_hello_world(self):
content = Content(
"text/plain", "and easy to do anywhere, even with Python")
mail = Mail(from_email, subject, to_email, content)
self.assertTrue(mail.get() == {'content': [{'type': 'text/plain', 'value': 'and easy to do anywhere, even with Python'}], 'personalizations': [
{'to': [{'email': '[email protected]'}]}], 'from': {'email': '[email protected]'}, 'subject': 'Sending with SendGrid is Fun'})
self.assertTrue(mail.get() == {
Copy link

Choose a reason for hiding this comment

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

self.assertEqual(mail.get, {...})  # ?

Copy link
Contributor Author

@vkmrishad vkmrishad Oct 30, 2018

Choose a reason for hiding this comment

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

@42B
I have updated file with master branch. This file was changed when merged with others PR.
May be this PR was at the time of github outage period.

mail.get() == {'content': [{'type': 'text/plain', 'value': 'and easy to do anywhere, even with Python'}],
'personalizations': [
{'to': [{'email': '[email protected]'}]}], 'from': {'email': '[email protected]'},
'subject': 'Sending with SendGrid is Fun'})
Copy link

Choose a reason for hiding this comment

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

It still seems like this should be changed from self.assertTrue to self.assertEqual? Maybe something like this?

self.assertEqual(mail.get(), {
    'content': [{
        'type': 'text/plain',
        'value': 'and easy to do anywhere, even with Python'
    }],
    'personalizations': [{
        'to': [{'email': '[email protected]'}]
    }],
    'from': {'email': '[email protected]'},
    'subject': 'Sending with SendGrid is Fun'
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@42B
You can create a new issue for this. I used the same code from master.

@thinkingserious thinkingserious added status: ready for deploy code ready to be released in next deploy status: hacktoberfest approved and removed status: work in progress Twilio or the community is in the process of implementing labels Oct 30, 2018
@thinkingserious
Copy link
Contributor

Hello @vkmrishad,

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 thinkingserious merged commit 2b6f07e into sendgrid:master Oct 30, 2018
@vkmrishad
Copy link
Contributor Author

@thinkingserious @misterdorm @42B
Thank you

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: ready for deploy code ready to be released in next deploy type: twilio enhancement feature request on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants