Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(714)

Issue 101330049: code review 101330049: encoding: added the quotedprintable package. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 9 months ago by alexcesaro
Modified:
9 years, 3 months ago
Visibility:
Public.

Description

encoding: added the quotedprintable package. Fixes issue 4943. Fixes issue 4687. Fixes issue 6611. Fixes issue 7079. Fixes issue 7140.

Patch Set 1 #

Patch Set 2 : diff -r ea9b86f1ad2d https://code.google.com/p/go/ #

Patch Set 3 : diff -r ea9b86f1ad2d https://code.google.com/p/go/ #

Patch Set 4 : diff -r ea9b86f1ad2d https://code.google.com/p/go/ #

Total comments: 8

Patch Set 5 : diff -r ea9b86f1ad2d https://code.google.com/p/go/ #

Total comments: 14

Patch Set 6 : diff -r ea9b86f1ad2d https://code.google.com/p/go/ #

Patch Set 7 : diff -r ea9b86f1ad2d https://code.google.com/p/go/ #

Total comments: 21

Patch Set 8 : diff -r ea9b86f1ad2d https://code.google.com/p/go/ #

Patch Set 9 : diff -r 42bcf6ae4aff0511890d4fe35aa15f14b7b751c8 https://code.google.com/p/go/ #

Total comments: 15

Patch Set 10 : diff -r 601ad8b625532388ff9dee86409bf32c1f2f624a https://code.google.com/p/go/ #

Patch Set 11 : diff -r 601ad8b625532388ff9dee86409bf32c1f2f624a https://code.google.com/p/go/ #

Total comments: 16

Patch Set 12 : diff -r 4085766e83907da960acf906f483372e1cce7130 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1175 lines, -464 lines) Patch
M src/pkg/go/build/deps_test.go View 1 2 3 4 5 6 7 8 9 10 2 chunks +38 lines, -37 lines 0 comments Download
A src/pkg/mime/header.go View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +409 lines, -0 lines 0 comments Download
A src/pkg/mime/header_test.go View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +115 lines, -0 lines 0 comments Download
A src/pkg/mime/internal/quotedprintable.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +39 lines, -0 lines 0 comments Download
M src/pkg/mime/multipart/multipart.go View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -1 line 0 comments Download
R src/pkg/mime/multipart/quotedprintable.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -118 lines 0 comments Download
R src/pkg/mime/multipart/quotedprintable_test.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -204 lines 0 comments Download
A src/pkg/mime/quotedprintable/quotedprintable.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +222 lines, -0 lines 0 comments Download
A src/pkg/mime/quotedprintable/quotedprintable_test.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +313 lines, -0 lines 0 comments Download
M src/pkg/net/mail/message.go View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +25 lines, -102 lines 0 comments Download
M src/pkg/net/mail/message_test.go View 1 2 3 4 5 6 7 8 9 10 3 chunks +12 lines, -2 lines 0 comments Download

Messages

Total messages: 40
alexcesaro
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
9 years, 9 months ago (2014-06-19 10:04:46 UTC) #1
ruiu
I think quotedprintable package is not the right place to put functions for =?charset?encoding?encoded-text?=. It ...
9 years, 9 months ago (2014-06-19 17:18:53 UTC) #2
alexcesaro
That is right but quoted-printable and Q encoding are very similar so if we do ...
9 years, 9 months ago (2014-06-19 22:56:50 UTC) #3
ruiu
Quoted printable is an encoding scheme, so I think encoding directory is the right place ...
9 years, 9 months ago (2014-06-20 00:29:34 UTC) #4
alexcesaro
After looking more closely, the only duplicated code would be the encodeByte function (since Q ...
9 years, 9 months ago (2014-06-20 15:02:14 UTC) #5
alexcesaro
On an unrelated matter, the Decode function would be much cleaner and simpler if the ...
9 years, 9 months ago (2014-06-20 15:08:47 UTC) #6
ruiu
On Fri, Jun 20, 2014 at 8:01 AM, Alexandre Cesaro < alexandre.cesaro@gmail.com> wrote: > After ...
9 years, 9 months ago (2014-06-20 19:44:28 UTC) #7
alexcesaro
I am not sure it is a good idea. It will make the API more ...
9 years, 9 months ago (2014-06-22 12:00:30 UTC) #8
alexcesaro
What do you think? On Sun, Jun 22, 2014 at 1:59 PM, Alexandre Cesaro < ...
9 years, 9 months ago (2014-07-01 10:03:25 UTC) #9
bradfitz
Sorry, this isn't a thorough review yet. Just looking at the API. I'm back to ...
9 years, 9 months ago (2014-07-01 16:22:07 UTC) #10
alexcesaro
Yes I also think this should be in the mime/quotedprintable package. Quoted-printable is not a ...
9 years, 9 months ago (2014-07-01 17:26:36 UTC) #11
alexcesaro
https://codereview.appspot.com/101330049/diff/60001/src/pkg/encoding/quotedprintable/quotedprintable.go File src/pkg/encoding/quotedprintable/quotedprintable.go (right): https://codereview.appspot.com/101330049/diff/60001/src/pkg/encoding/quotedprintable/quotedprintable.go#newcode25 src/pkg/encoding/quotedprintable/quotedprintable.go:25: func Encode(dst, src []byte) (n int) { On 2014/07/01 ...
9 years, 8 months ago (2014-07-07 13:29:03 UTC) #12
bradfitz
https://codereview.appspot.com/101330049/diff/80001/src/pkg/encoding/quotedprintable/header.go File src/pkg/encoding/quotedprintable/header.go (right): https://codereview.appspot.com/101330049/diff/80001/src/pkg/encoding/quotedprintable/header.go#newcode22 src/pkg/encoding/quotedprintable/header.go:22: Q = "Q" let's make these proper types. type ...
9 years, 8 months ago (2014-07-09 20:15:06 UTC) #13
alexcesaro
https://codereview.appspot.com/101330049/diff/80001/src/pkg/encoding/quotedprintable/header.go File src/pkg/encoding/quotedprintable/header.go (right): https://codereview.appspot.com/101330049/diff/80001/src/pkg/encoding/quotedprintable/header.go#newcode22 src/pkg/encoding/quotedprintable/header.go:22: Q = "Q" On 2014/07/09 20:15:06, bradfitz wrote: > ...
9 years, 8 months ago (2014-07-16 17:24:58 UTC) #14
alexcesaro
On Wed, Jul 16, 2014 at 7:24 PM, <alexandre.cesaro@gmail.com> wrote: > > https://codereview.appspot.com/101330049/diff/80001/src/pkg/encoding/quotedprintable/header.go#newcode77 > src/pkg/encoding/quotedprintable/header.go:77: ...
9 years, 8 months ago (2014-07-17 15:20:07 UTC) #15
alexcesaro
There is a caveat to return []byte. Encode currently returns the input if the string ...
9 years, 8 months ago (2014-07-17 15:42:04 UTC) #16
alexcesaro
Did you get a chance to have a look? I've got some time on my ...
9 years, 7 months ago (2014-08-20 10:13:26 UTC) #17
josharian
Here are some drive-by comments and nits. https://codereview.appspot.com/101330049/diff/120001/src/pkg/encoding/quotedprintable/header.go File src/pkg/encoding/quotedprintable/header.go (right): https://codereview.appspot.com/101330049/diff/120001/src/pkg/encoding/quotedprintable/header.go#newcode31 src/pkg/encoding/quotedprintable/header.go:31: func (enc ...
9 years, 7 months ago (2014-08-20 21:16:58 UTC) #18
alexcesaro
Thank you. I just did the changes. Except those: https://codereview.appspot.com/101330049/diff/120001/src/pkg/encoding/quotedprintable/header.go File src/pkg/encoding/quotedprintable/header.go (right): https://codereview.appspot.com/101330049/diff/120001/src/pkg/encoding/quotedprintable/header.go#newcode31 src/pkg/encoding/quotedprintable/header.go:31: ...
9 years, 7 months ago (2014-08-21 15:31:35 UTC) #19
josharian
> https://codereview.appspot.com/101330049/diff/120001/src/pkg/encoding/quotedprintable/header.go#newcode31 > src/pkg/encoding/quotedprintable/header.go:31: func (enc *Encoding) equals(e > Encoding) bool { > On 2014/08/20 ...
9 years, 7 months ago (2014-08-21 18:13:57 UTC) #20
alexcesaro
On Thu, Aug 21, 2014 at 8:13 PM, <josharian@gmail.com> wrote: > > https://codereview.appspot.com/101330049/diff/120001/src/ > pkg/encoding/quotedprintable/header.go#newcode31 ...
9 years, 7 months ago (2014-08-22 14:12:29 UTC) #21
ruiu
https://codereview.appspot.com/101330049/diff/160001/src/pkg/encoding/quotedprintable/header.go File src/pkg/encoding/quotedprintable/header.go (right): https://codereview.appspot.com/101330049/diff/160001/src/pkg/encoding/quotedprintable/header.go#newcode26 src/pkg/encoding/quotedprintable/header.go:26: Q Encoding = "Q" It's odd if B encoding ...
9 years, 7 months ago (2014-08-23 00:31:32 UTC) #22
alexcesaro
On Sat, Aug 23, 2014 at 2:31 AM, <ruiu@google.com> wrote: > > https://codereview.appspot.com/101330049/diff/160001/src/ > pkg/encoding/quotedprintable/header.go ...
9 years, 7 months ago (2014-08-25 12:10:24 UTC) #23
bradfitz
https://codereview.appspot.com/101330049/diff/160001/src/pkg/encoding/quotedprintable/header.go File src/pkg/encoding/quotedprintable/header.go (right): https://codereview.appspot.com/101330049/diff/160001/src/pkg/encoding/quotedprintable/header.go#newcode31 src/pkg/encoding/quotedprintable/header.go:31: func (enc *Encoding) equals(e Encoding) bool { I don't ...
9 years, 7 months ago (2014-08-25 17:08:35 UTC) #24
bradfitz
I think mime/headers is probably too small of a package. We can always put shared ...
9 years, 7 months ago (2014-08-25 17:10:17 UTC) #25
ruiu
How about moving header.go under mime as a part of package mime? On Mon, Aug ...
9 years, 7 months ago (2014-08-25 17:36:19 UTC) #26
bradfitz
That sounds fine. On Mon, Aug 25, 2014 at 10:35 AM, Rui Ueyama <ruiu@google.com> wrote: ...
9 years, 7 months ago (2014-08-25 17:38:18 UTC) #27
alexcesaro
Ok, so: - quoted-printable code in package mime/quotedprintable - RFC 2047 code in header.go in ...
9 years, 7 months ago (2014-08-26 17:32:00 UTC) #28
bradfitz
The tree closes for non-bugfix changes September 1st. If it's not in by then, it'll ...
9 years, 7 months ago (2014-08-26 17:35:39 UTC) #29
alexcesaro
Ok I will try to do it tomorrow. On Tue, Aug 26, 2014 at 7:35 ...
9 years, 7 months ago (2014-08-26 17:39:20 UTC) #30
ruiu
On Tue, Aug 26, 2014 at 10:31 AM, Alexandre Cesaro < alexandre.cesaro@gmail.com> wrote: > Ok, ...
9 years, 7 months ago (2014-08-26 19:01:49 UTC) #31
alexcesaro
https://codereview.appspot.com/101330049/diff/160001/src/pkg/encoding/quotedprintable/header.go File src/pkg/encoding/quotedprintable/header.go (right): https://codereview.appspot.com/101330049/diff/160001/src/pkg/encoding/quotedprintable/header.go#newcode64 src/pkg/encoding/quotedprintable/header.go:64: return &HeaderEncoder{charset, enc, splitWords}, nil On 2014/08/25 17:08:34, bradfitz ...
9 years, 7 months ago (2014-08-27 09:55:56 UTC) #32
alexcesaro
I did the changes. I had to update build/deps_test.go. That is why I removed the ...
9 years, 7 months ago (2014-08-27 11:20:58 UTC) #33
ruiu
https://codereview.appspot.com/101330049/diff/240001/src/pkg/mime/header.go File src/pkg/mime/header.go (right): https://codereview.appspot.com/101330049/diff/240001/src/pkg/mime/header.go#newcode33 src/pkg/mime/header.go:33: type HeaderEncoder struct { Is "header word" a correct ...
9 years, 7 months ago (2014-08-28 04:02:16 UTC) #34
alexcesaro
Here are some comments. I am on my phone, sorry if my messages are short. ...
9 years, 7 months ago (2014-08-28 14:00:51 UTC) #35
alexcesaro
I did the changes. https://codereview.appspot.com/101330049/diff/240001/src/pkg/mime/quotedprintable/quotedprintable.go File src/pkg/mime/quotedprintable/quotedprintable.go (right): https://codereview.appspot.com/101330049/diff/240001/src/pkg/mime/quotedprintable/quotedprintable.go#newcode1 src/pkg/mime/quotedprintable/quotedprintable.go:1: // Copyright 2014 The Go ...
9 years, 7 months ago (2014-09-01 12:31:44 UTC) #36
alexcesaro
I closed this CL an opened a new one because of a bug due to ...
9 years, 6 months ago (2014-09-15 16:20:57 UTC) #37
bradfitz
Unfortunately this was too late and combined with my vacation immediately after dotGo travels, pushed ...
9 years, 4 months ago (2014-11-12 18:13:09 UTC) #38
alexcesaro
We now are early december and the Go 1.5 dev cycle is beginning so it ...
9 years, 3 months ago (2014-12-03 10:48:10 UTC) #39
bradfitz
9 years, 3 months ago (2014-12-18 03:33:04 UTC) #40
The tree is now open. See
https://groups.google.com/forum/#!topic/golang-dev/otCULnOjs7I for details
on how to re-send this using our new git-based process.


On Wed, Dec 3, 2014 at 9:47 PM, Alexandre Cesaro <alexandre.cesaro@gmail.com
> wrote:
>
> We now are early december and the Go 1.5 dev cycle is beginning so it is
> time to get back at this CL.
>
> As I said earlier I closed this CL due to the big package move and opened
> a new one: https://codereview.appspot.com/132680044/
>
>
> On Wed, Nov 12, 2014 at 7:13 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
>
>> Unfortunately this was too late and combined with my vacation immediately
>> after dotGo travels, pushed it past the deadline.
>>
>> Let's pick this up in early December so it has time to bake during the
>> three-month Go 1.5 dev cycle.
>>
>>
>> On Mon, Sep 1, 2014 at 9:31 AM, <alexandre.cesaro@gmail.com> wrote:
>>
>>> I did the changes.
>>>
>>>
>>> https://codereview.appspot.com/101330049/diff/240001/src/
>>> pkg/mime/quotedprintable/quotedprintable.go
>>> File src/pkg/mime/quotedprintable/quotedprintable.go (right):
>>>
>>> https://codereview.appspot.com/101330049/diff/240001/src/
>>> pkg/mime/quotedprintable/quotedprintable.go#newcode1
>>> src/pkg/mime/quotedprintable/quotedprintable.go:1: // Copyright 2014 The
>>> Go Authors. All rights reserved.
>>> On 2014/08/28 04:02:16, ruiu wrote:
>>>
>>>> Now this file is not related to mime, you can move it to
>>>> src/pkg/encoding/quotedprintable.
>>>>
>>>
>>> I thought we had settled on the following organization:
>>>  - quoted-printable code in package mime/quotedprintable
>>>  - RFC 2047 code in header.go in package mime
>>>  - shared code in mime/internal
>>>
>>> Quoted-printable encoding is defined in the MIME specification. It also
>>> shares some code with mime/header.go: mime/internal/quotedprintable.go
>>> That is why I think it should stay in the mime package.
>>>
>>> https://codereview.appspot.com/101330049/
>>>
>>
>>
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b