-
Notifications
You must be signed in to change notification settings - Fork 4
Sendgrid Plugins #9
Conversation
| IdUtils.validateReferenceName(config.referenceName, failureCollector); | ||
|
|
||
| config.validate(failureCollector); | ||
| config.validate(pipelineConfigurer.getStageConfigurer().getInputSchema()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Input schema can be null if the previous stage's output schema is a macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try testing with a pipeline where schema fields are macro params. You could try something like source -> wrangler -> sendgrid sink
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO should be in the code, not a comment on the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code updated with ToDO and created JIRA issue https://issues.cask.co/browse/PLUGIN-359
| } | ||
|
|
||
| /** | ||
| * Returns the string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please write a more descriptive comment.
Also, @return string doesn't tell us what is being returned. We already know that the return type is String.
Similar comment in other places in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/main/java/io/cdap/plugin/sendgrid/batch/sink/SendGridSinkConfigValidator.java
Show resolved
Hide resolved
| } | ||
|
|
||
| private void checkReplyTo() { | ||
| if (!Strings.isNullOrEmpty(config.getReplyTo()) && validateEmail(config.getReplyTo())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this the opposite of checkFrom?
if (!Strings.isNullOrEmpty(config.getReplyTo()) && validateEmail(config.getReplyTo()))
vs
if (Strings.isNullOrEmpty(config.getFrom()) || !validateEmail(config.getFrom()))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is opposite because ReplyTo config is not mandatory. Moreover, we found another bug in the same validation code so fixed that too.
…nk) (data-integrations#2) Co-authored-by: Mikkin Patel <mikkin@google.com> Fixed an issue with macros not working Added missing javadocs Changed copyright to 2020 Modified to implement PR comments Added TODO in code and JIRA ticket ID
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Fixed an issue with macros not working
Added missing javadocs
Changed copyright to 2020