Hi
I made some changes and made a few new classes to allow rudimentary creation and sending of multipart message using only Seaside project classes. Where should I post the code for people to review and consider adopting? Thanks Paul_______________________________________________ seaside-dev mailing list [hidden email] http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev |
I put the packages here:
MCHttpRepository location: 'http://smalltalkhub.com/mc/Seaside/Seaside30/main' user: '' password: '' On Oct 25, 2013, at 12:15 PM, Paul DeBruicker <[hidden email]> wrote: > Hi > > I made some changes and made a few new classes to allow rudimentary creation and sending of multipart message using only Seaside project classes. > > Where should I post the code for people to review and consider adopting? > > > Thanks > > Paul _______________________________________________ seaside-dev mailing list [hidden email] http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev |
On Fri, Oct 25, 2013 at 10:53 PM, Paul DeBruicker <[hidden email]> wrote:
> I put the packages here: > > > MCHttpRepository > location: 'http://smalltalkhub.com/mc/Seaside/Seaside30/main' > user: '' > password: '' Thank you, I'll have a look. Cheers Philippe _______________________________________________ seaside-dev mailing list [hidden email] http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev |
On Sat, Oct 26, 2013 at 2:16 PM, Philippe Marschall
<[hidden email]> wrote: > On Fri, Oct 25, 2013 at 10:53 PM, Paul DeBruicker <[hidden email]> wrote: >> I put the packages here: >> >> >> MCHttpRepository >> location: 'http://smalltalkhub.com/mc/Seaside/Seaside30/main' >> user: '' >> password: '' > > Thank you, I'll have a look. First, thank you again. I'm going to be a bit picky, that's not because your contribution isn't welcome. - Can you use GRPlatfrom current newRandom instead of Random new? (Slime should have picked that one up) - Can you name MyRandom something else? Like BoundaryRandomGenerator or something similar? - Wan you use WAMimeType instead of Strings? - Why does WAEmailBody implement #= and #<=? - Why do you sort the parts? - You hard code charset=UTF-8 (as a String) you I don't see you doing any encoding anywhere. - I wonder whether it would make sense to have a WASimpleEmailMessage (subclass of WAEmailMessage) where the body is just a String and get rid of WAStringEmailBody is this case (but we would actually need an encoding in that case as well). Or combine WAMultiPartEmailMessage and WAEmailMessage into a single class. - Can we get rid of #htmlBodyString and #plainTextBodyString and instead delegate more to the parts? - Can we find a better replacement for the #string selector? Cheers Philippe _______________________________________________ seaside-dev mailing list [hidden email] http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev |
Hi Philippe,
I finally had time to get back to this. Thanks for the review. Comments below. On Nov 3, 2013, at 12:53 PM, Philippe Marschall <[hidden email]> wrote: > On Sat, Oct 26, 2013 at 2:16 PM, Philippe Marschall > <[hidden email]> wrote: >> On Fri, Oct 25, 2013 at 10:53 PM, Paul DeBruicker <[hidden email]> wrote: >>> I put the packages here: >>> >>> >>> MCHttpRepository >>> location: 'http://smalltalkhub.com/mc/Seaside/Seaside30/main' >>> user: '' >>> password: '' >> >> Thank you, I'll have a look. > > First, thank you again. I'm going to be a bit picky, that's not > because your contribution isn't welcome. > - Can you use GRPlatfrom current newRandom instead of Random new? > (Slime should have picked that one up) fixed > - Can you name MyRandom something else? Like BoundaryRandomGenerator > or something similar? Fixed. MyRandom was left over from an earlier experiment in generating a random boundary. I removed it. > - Wan you use WAMimeType instead of Strings? Fixed. > - Why does WAEmailBody implement #= and #<=? > - Why do you sort the parts? I would not be surprised to learn I was wrong about _needing_ to do this but here's why I did: I was following this spec (http://www.w3.org/Protocols/rfc1341/7_2_Multipart.html). I chose to use the 'multipart/alternative' subtype described in heading 7.2.3 of the spec linked above. It recommends sorting the parts from plainest to 'richest'. So thats why I implemented #<= and sort the parts. I just assume that when using 'multipart/alternative' it may be problematic for some clients to process emails that have more than one part with the same type, so I use a Set to store the parts and implement #=. At some point (maybe now?) WAMultiPartEmailMessage should be changed so the primary Content-Type: of the message is multipart/mixed and any text/html email bodies are included in a subpart of Content-Type: multipart/alternative. That would allow other useful parts to be included (e.g. attachments, .ics files, etc) in the main message. Right now, i think, there is no way to add an attachment alongside the readable email body. > - You hard code charset=UTF-8 (as a String) you I don't see you doing > any encoding anywhere. I was just aping other implementations. But if you leave it off or change it to charset=us-ascii then the email is not processed by Mail.app as a multipart email. That may be because I'm using multipart/alternative as the main Content-Type: of the email rather than multipart/mixed. I'll check it out. > - I wonder whether it would make sense to have a WASimpleEmailMessage > (subclass of WAEmailMessage) where the body is just a String and get > rid of WAStringEmailBody is this case (but we would actually need an > encoding in that case as well). Or combine WAMultiPartEmailMessage and > WAEmailMessage into a single class. Combining them may not be too bad, but the Multipart implementation right now is simple and doesn't include attachments as described above. I'm not sure whether to combine them now or wait and have a look once the other multipart parts work . > - Can we get rid of #htmlBodyString and #plainTextBodyString and > instead delegate more to the parts? I moved them to be extensions to the PostMark-Seaside package as that's where I'm using them. http://postmarkapp.com a service that provides mail deliverability and wants emails in JSON format. > - Can we find a better replacement for the #string selector? > I renamed it to #contentString > Cheers > Philippe > _______________________________________________ > seaside-dev mailing list > [hidden email] > http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev Thanks and let me know what else should get fixed next. Do you want me to put this revised version somewhere? Paul_______________________________________________ seaside-dev mailing list [hidden email] http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev |
On Nov 6, 2013, at 4:30 PM, Paul DeBruicker <[hidden email]> wrote: >> - You hard code charset=UTF-8 (as a String) you I don't see you doing >> any encoding anywhere. > > I was just aping other implementations. But if you leave it off or change it to charset=us-ascii then the email is not processed by Mail.app as a multipart email. That may be because I'm using multipart/alternative as the main Content-Type: of the email rather than multipart/mixed. I'll check it out. The above is wrong. You can leave off the charset parameter and Mail.app (and others) has no problem opening it. _______________________________________________ seaside-dev mailing list [hidden email] http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev |
In reply to this post by Paul DeBruicker
Hi Paul,
Did you have a chance to commit these changes? The repository, I only see your earlier package version. Thank you! Johan On 07 Nov 2013, at 01:30, Paul DeBruicker <[hidden email]> wrote: > Hi Philippe, > > I finally had time to get back to this. Thanks for the review. Comments below. > > > On Nov 3, 2013, at 12:53 PM, Philippe Marschall <[hidden email]> wrote: > >> On Sat, Oct 26, 2013 at 2:16 PM, Philippe Marschall >> <[hidden email]> wrote: >>> On Fri, Oct 25, 2013 at 10:53 PM, Paul DeBruicker <[hidden email]> wrote: >>>> I put the packages here: >>>> >>>> >>>> MCHttpRepository >>>> location: 'http://smalltalkhub.com/mc/Seaside/Seaside30/main' >>>> user: '' >>>> password: '' >>> >>> Thank you, I'll have a look. >> >> First, thank you again. I'm going to be a bit picky, that's not >> because your contribution isn't welcome. >> - Can you use GRPlatfrom current newRandom instead of Random new? >> (Slime should have picked that one up) > > fixed > >> - Can you name MyRandom something else? Like BoundaryRandomGenerator >> or something similar? > > Fixed. MyRandom was left over from an earlier experiment in generating a random boundary. I removed it. > >> - Wan you use WAMimeType instead of Strings? > > Fixed. > >> - Why does WAEmailBody implement #= and #<=? >> - Why do you sort the parts? > > I would not be surprised to learn I was wrong about _needing_ to do this but here's why I did: > > I was following this spec (http://www.w3.org/Protocols/rfc1341/7_2_Multipart.html). I chose to use the 'multipart/alternative' subtype described in heading 7.2.3 of the spec linked above. It recommends sorting the parts from plainest to 'richest'. So thats why I implemented #<= and sort the parts. I just assume that when using 'multipart/alternative' it may be problematic for some clients to process emails that have more than one part with the same type, so I use a Set to store the parts and implement #=. > > At some point (maybe now?) WAMultiPartEmailMessage should be changed so the primary Content-Type: of the message is multipart/mixed and any text/html email bodies are included in a subpart of Content-Type: multipart/alternative. That would allow other useful parts to be included (e.g. attachments, .ics files, etc) in the main message. Right now, i think, there is no way to add an attachment alongside the readable email body. > > > > >> - You hard code charset=UTF-8 (as a String) you I don't see you doing >> any encoding anywhere. > > I was just aping other implementations. But if you leave it off or change it to charset=us-ascii then the email is not processed by Mail.app as a multipart email. That may be because I'm using multipart/alternative as the main Content-Type: of the email rather than multipart/mixed. I'll check it out. > > > >> - I wonder whether it would make sense to have a WASimpleEmailMessage >> (subclass of WAEmailMessage) where the body is just a String and get >> rid of WAStringEmailBody is this case (but we would actually need an >> encoding in that case as well). Or combine WAMultiPartEmailMessage and >> WAEmailMessage into a single class. > > Combining them may not be too bad, but the Multipart implementation right now is simple and doesn't include attachments as described above. I'm not sure whether to combine them now or wait and have a look once the other multipart parts work . > >> - Can we get rid of #htmlBodyString and #plainTextBodyString and >> instead delegate more to the parts? > > I moved them to be extensions to the PostMark-Seaside package as that's where I'm using them. http://postmarkapp.com a service that provides mail deliverability and wants emails in JSON format. > > >> - Can we find a better replacement for the #string selector? >> > > I renamed it to #contentString > >> Cheers >> Philippe >> _______________________________________________ >> seaside-dev mailing list >> [hidden email] >> http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev > > > Thanks and let me know what else should get fixed next. Do you want me to put this revised version somewhere? > > Paul_______________________________________________ > seaside-dev mailing list > [hidden email] > http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev _______________________________________________ seaside-dev mailing list [hidden email] http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev |
Oops. I forgot. They're there now. Thanks for the reminder Paul |
Free forum by Nabble | Edit this page |