search

genkgo / mail

Library to send e-mails over different transports and protocols (like SMTP and IMAP) using immutable messages and streams. Also includes SMTP server.
https://mail.readthedocs.io/
Other
402 stars 21 forks source link

External code review #2

Closed frederikbosch closed 7 years ago

frederikbosch commented 7 years ago

Have code reviewed by someone using similar standards and code style.

frederikbosch commented 7 years ago

@sagikazarmark You are a busy man, so I am only asking this in the event you have some time left, could you do an initial review of this library?

At this stage I am pretty happy with the state. But as experienced with Money: a second pair of eyes is required to reach the extraordinary.

sagikazarmark commented 7 years ago

Sure thing, I will give it a look over the weekend.

sagikazarmark commented 7 years ago

Did you have any specific reason to write a new one? There are a few already out there (like Swift mailer). Are there any specific new features present in this library?

frederikbosch commented 7 years ago

Swift: no namespaces, scalar property bag. It is legacy all over the place. Inheritance hell. Not a particular easy api.

Same with zend/mail: public properties, no value objects, hard to construct messages that have html, alternative text and attachments. Furthermore, functionality scattered all over the place.

My decision to write this: simply saying no to extreme legacy in our codebase.

frederikbosch commented 7 years ago

Thanks for your effort!!

pachico commented 7 years ago

This is the first mail lib that, when looking at the source code, doesn't make my eyes hurt. Great work!

frederikbosch commented 7 years ago

Thanks for your kind words!

sagikazarmark commented 7 years ago

I was finally able to find some time to review the library. I didn't go into too much details, I rather looked at it from a developer/user point of view and I have to say I can't agree more with @pachico . The API is indeed great from a DX and a design point of view. The feature set and RFC compliance is also quite impressing.

Good job @frederikbosch , I can't wait to try it out.

If you plan to add more transports/protocols I would probably move the project to it's own organization, find it a good name, etc. It's not necessary, but in my experience "generic" organizations rarely serve a good place for projects like this, they usually simply "outgrow" them.

frederikbosch commented 7 years ago

Thanks for your review and advice @sagikazarmark. I will do my best to keep the package up to standards and add more functionality.