Add types and specs #150
Add types and specs #150
Conversation
cloud8421
force-pushed
the
add-types-and-specs
branch
from
d2330bd
to
997eb62
Compare
| @@ -58,6 +58,8 @@ defprotocol Bamboo.Formatter do | |||
| `:from`, `:to`, `:cc` or `:bcc`. You can pattern match on this to customize | |||
| the address. | |||
| """ | |||
| @type opts :: %{type: :from | :to | :cc | :bcc} | |||
There was a problem hiding this comment.
a newline after this, maybe, for readability?
There was a problem hiding this comment.
Yes I think this would make it read a bit better
|
@paulcsmith have you missed this goodie? |
|
@cloud8421 out of curiosity, how did you generate your daily digest? did you use any task queue / cron? |
|
Did not miss it, just been busy in client work :D Thanks so much for this @cloud8421. I'm glad you're liking it so far! I'll have more time to review this later on in the week :) @princemaple FWIW, we do a daily digest and use Heroku Scheduler with a mix task and it works wonderfully, but you need to be on Heroku. Not sure if that helps, but hopefully it does! |
|
@paulcsmith thanks for the information :) |
|
With those requirements you Kay be interested in using a package called Quantum It's basically cron, but pure elixir. We have used it on a client project and it is working well so far. You can even set schedules with human readable language which is nice, e.g. "@hourly" On phone so don't have a link but it's on hex :)
|
|
Thanks, that one is interesting. |
|
@paulcsmith: sounds great! The project I’m working on is a system that gathers information in the background and reacts to certain events.
Both consumer and producers are Everything runs in the same application. Hope it's clear! |
|
@cloud8421 yep, that's clear enough :) thank you! |
| @type t :: %__MODULE__{ | ||
| to: nil | address | [address], | ||
| cc: nil | address | [address], | ||
| bcc: nil | address | [address], |
There was a problem hiding this comment.
I think this should be nil | any because the email address can technically be whatever you want as long as it defines the Bamboo.Formatter protocol
There was a problem hiding this comment.
Of course, that's true.
|
A few comments and then this will be ready to merge. Thanks @cloud8421 |
|
@paulcsmith will make the changes and push. Is there a policy around squashing/rebasing? |
|
@cloud8421 I typically squash the commits before merging, but I can't easily rebase it from the web. So if you can rebase I can manage squashing/merging. Does that work for you? |
|
@cloud8421 Just checking in to see if you need any help with this :) |
|
Yes I think I'll be able to make those changes later today Claudio Sent from iPhone
|
cloud8421
force-pushed
the
add-types-and-specs
branch
from
997eb62
to
ea474ac
Compare
- All address types can be `any`, as they depend on the `Bamboo.Formatter` protocol - `Email.put_private/3` can accept `any` as a value
Methods are generated by a macro, however their specs are different.
|
@paulcsmith rebased, made needed changes and left a question :) |
|
This looks great. Thanks @cloud8421! |
What changed? ============ In #386, evuez pointed out that the `Bamboo.Email.address()` typespec shows `String.t() | {String.t(), String.t()}`. But no adapter actually supports the `String.t()` type. Given that the code was introduced [before the typespecs](#150) (and thus we can treat the code as the source of truth when in doubt), and since `Bamboo.Email.get_address/1`'s explicit intent is to abstract the internal representation of email addresses in case Bamboo changes how it stores them: > Gets just the email address from a normalized email address. > Normalized email addresses are 2 item tuples {name, address}. This gets the address part of the tuple. Use this instead of calling elem(address, 1) so that if Bamboo changes how email addresses are represented your code will still work I think the correct change here is to remove the `String.t()` portion of the typespec rather than update the code to handle that typespec. In other words, I think we made a typo when introducing the typespec. So this PR would be merged instead of #386.
What changed? ============ In #386, evuez pointed out that the `Bamboo.Email.address()` typespec shows `String.t() | {String.t(), String.t()}`. But no adapter actually supports the `String.t()` type. Given that the code was introduced [before the typespecs](#150) (and thus we can treat the code as the source of truth when in doubt), and since `Bamboo.Email.get_address/1`'s explicit intent is to abstract the internal representation of email addresses in case Bamboo changes how it stores them: > Gets just the email address from a normalized email address. > Normalized email addresses are 2 item tuples {name, address}. This gets the address part of the tuple. Use this instead of calling elem(address, 1) so that if Bamboo changes how email addresses are represented your code will still work I think the correct change here is to remove the `String.t()` portion of the typespec rather than update the code to handle that typespec. In other words, I think we made a typo when introducing the typespec. So this PR would be merged instead of #386.
Hello, I've added some initial types and specs (excluding the Plug/Phoenix side of things).
Incidentally, I've been using the library for a few weeks now (just a to generate a daily digest to myself) and it works very nicely, I'm really happy about it! Nice job.