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.

Sign up for GitHub

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

Jump to bottom

Go lint fixes #121

Merged
merged 3 commits into from
Oct 22, 2017
Merged

Go lint fixes #121

merged 3 commits into from
Oct 22, 2017

Conversation

srini156
Copy link
Contributor

@srini156 srini156 commented Oct 3, 2017 •
edited by thinkingserious
Loading

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

vvakame commented Oct 3, 2017

FYI: https://github.com/sendgrid/sendgrid-go/pull/121/files?w=1

@leandro-lugaresi
Copy link
Contributor

:shipit:

@thinkingserious thinkingserious added difficulty: medium fix is medium in difficulty hacktoberfest status: code review request requesting a community code review or review from Twilio labels Oct 4, 2017
@leandro-lugaresi leandro-lugaresi mentioned this pull request Oct 9, 2017
Merged
andy-trimble
andy-trimble suggested changes Oct 12, 2017
View reviewed changes
Copy link
Contributor

@andy-trimble andy-trimble left a comment

Choose a reason for hiding this comment

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

There are a lot of golint errors of the form exported method ... should have comment or be unexported. Please fix these as well. In general, the comments in the files are of the wrong form.

@srini156
Copy link
Contributor Author

@andy-trimble - I haven't fixed go lint violations. I will definitely have a look.

@tariq1890 tariq1890 mentioned this pull request Oct 14, 2017
Merged
mbernier
mbernier reviewed Oct 22, 2017
View reviewed changes
helpers/mail/mail_v3_test.go
@@ -63,7 +63,7 @@ func TestV3AddPersonalizations(t *testing.T) {
m.AddPersonalizations(personalizations...)

if len(m.Personalizations) != numOfPersonalizations {
t.Errorf("Mail should have %d personalizations, got %d personalizations", personalizations, len(m.Personalizations))
t.Errorf("Mail should have %d personalizations, got %d personalizations", len(personalizations), len(m.Personalizations))
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you use len(personalizations)

but on lines 81 and 96, it looks like you used the numOf* variable. Which is preferrable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry @mbernier didn't check this review comment. Seems the changes are now merged. Will clean this up in later PR.

@mbernier mbernier mentioned this pull request Oct 22, 2017
Closed
@mbernier
Copy link
Contributor

I am going to merge this in and make a new PR for the golint issues. Thanks everyone!

@mbernier mbernier merged commit 81101c1 into sendgrid:master Oct 22, 2017
@thinkingserious
Copy link
Contributor

Hello @srini156,

Thanks again for the PR!

We want to show our appreciation by sending you some swag. Could you please fill out this form so we can send it to you? Thanks!

Team SendGrid DX

@joepurdy joepurdy mentioned this pull request Oct 22, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andy-trimble andy-trimble andy-trimble requested changes

@mbernier mbernier mbernier approved these changes

Assignees
No one assigned
Labels
difficulty: medium fix is medium in difficulty status: code review request requesting a community code review or review from Twilio
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@srini156 @vvakame @leandro-lugaresi @mbernier @thinkingserious @andy-trimble