code review 87660043: net/html: Synchronize comments with code

net/html: Synchronize comments with code

[s.jegen, 2014-04-14 16:32:36]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[s.jegen, 2014-04-14 16:38:19]

The description should read "net/http: Synchronize comments with code", sorry
for that.

[bradfitz, 2014-04-14 19:58:00]

Doesn't seem worth it. Server requests may still be HTTP/1.0 anyway.

Also, CL subject says "net/html" but you mean "net/http".




On Mon, Apr 14, 2014 at 9:32 AM, <s.jegen@gmail.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net/html: Synchronize comments with code
>
> Please review this at https://codereview.appspot.com/87660043/
>
> Affected files (+2, -2 lines):
>   M src/pkg/net/http/request.go
>
>
> Index: src/pkg/net/http/request.go
> ===================================================================
> --- a/src/pkg/net/http/request.go
> +++ b/src/pkg/net/http/request.go
> @@ -94,9 +94,9 @@
>
>         // The protocol version for incoming requests.
>         // Client requests always use HTTP/1.1.
> -       Proto      string // "HTTP/1.0"
> +       Proto      string // "HTTP/1.1"
>         ProtoMajor int    // 1
> -       ProtoMinor int    // 0
> +       ProtoMinor int    // 1
>
>         // A header maps request lines to their values.
>         // If the header says
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[s.jegen, 2014-04-14 20:35:31]

On Mon, Apr 14, 2014 at 12:57:59PM -0700, Brad Fitzpatrick wrote:
> Doesn't seem worth it. Server requests may still be HTTP/1.0 anyway.

Do you mean server responses? That may be, but the requests will always
be HTTP/1.1 I think:

https://code.google.com/p/go/source/browse/src/pkg/net/http/request.go#488


Also,

http://golang.org/project/ bottom

> We pride ourselves on being meticulous; no issue is too small.

:-)


> Also, CL subject says "net/html" but you mean "net/http".

Yes, I noticed right after sending the CL. Sorry for that.

If you don't want to bother, please feel free to close this CL.
Otherwise I can resend with a corrected description (if the system
does not allow you to amend the description yourself).


Cheers,

Silvan


> On Mon, Apr 14, 2014 at 9:32 AM, <s.jegen@gmail.com> wrote:
> 
> > Reviewers: golang-codereviews,
> >
> > Message:
> > Hello golang-codereviews@googlegroups.com,
> >
> > I'd like you to review this change to
> > https://code.google.com/p/go
> >
> >
> > Description:
> > net/html: Synchronize comments with code
> >
> > Please review this at https://codereview.appspot.com/87660043/
> >
> > Affected files (+2, -2 lines):
> >   M src/pkg/net/http/request.go
> >
> >
> > Index: src/pkg/net/http/request.go
> > ===================================================================
> > --- a/src/pkg/net/http/request.go
> > +++ b/src/pkg/net/http/request.go
> > @@ -94,9 +94,9 @@
> >
> >         // The protocol version for incoming requests.
> >         // Client requests always use HTTP/1.1.
> > -       Proto      string // "HTTP/1.0"
> > +       Proto      string // "HTTP/1.1"
> >         ProtoMajor int    // 1
> > -       ProtoMinor int    // 0
> > +       ProtoMinor int    // 1
> >
> >         // A header maps request lines to their values.
> >         // If the header says
> >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "golang-codereviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to golang-codereviews+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/d/optout.
> >

[bradfitz, 2014-04-14 20:42:37]

On Mon, Apr 14, 2014 at 1:35 PM, Silvan Jegen <s.jegen@gmail.com> wrote:

> On Mon, Apr 14, 2014 at 12:57:59PM -0700, Brad Fitzpatrick wrote:
> > Doesn't seem worth it. Server requests may still be HTTP/1.0 anyway.
>
> Do you mean server responses? That may be, but the requests will always
> be HTTP/1.1 I think:
>
>
https://code.google.com/p/go/source/browse/src/pkg/net/http/request.go#48<htt...


No, requests to the server.  The Request struct is used for both outgoing
client requests, and incoming server requests.  A non-Go HTTP client can
set an "HTTP/1.0" ProtoMajor: 1 ProtoMinor: 0 request to a Go server.

So the comment you're changing is a valid case, hence my thinking the
change is arbitrary.

[s.jegen, 2014-04-14 21:06:01]

On Mon, Apr 14, 2014 at 01:42:36PM -0700, Brad Fitzpatrick wrote:
> On Mon, Apr 14, 2014 at 1:35 PM, Silvan Jegen <s.jegen@gmail.com> wrote:
> 
> > On Mon, Apr 14, 2014 at 12:57:59PM -0700, Brad Fitzpatrick wrote:
> > > Doesn't seem worth it. Server requests may still be HTTP/1.0 anyway.
> >
> > Do you mean server responses? That may be, but the requests will always
> > be HTTP/1.1 I think:
> >
> >
https://code.google.com/p/go/source/browse/src/pkg/net/http/request.go#48<htt...
> 
> 
> No, requests to the server.  The Request struct is used for both outgoing
> client requests, and incoming server requests.  A non-Go HTTP client can
> set an "HTTP/1.0" ProtoMajor: 1 ProtoMinor: 0 request to a Go server.

Ah, I see what you mean now.


> So the comment you're changing is a valid case, hence my thinking the
> change is arbitrary.

Having the line

// Client requests always use HTTP/1.1

immediately precede a contradicting comment seemed suspicious to me.
Maybe it would be better to remove the

// "HTTP/1.0"
// 1
// 0

comments since these are the only "annotated" fields anyways.

Thanks for clearing that up and taking the time to respond. Please feel
free to close the CL. It has been a misunderstanding on my part.

[bradfitz, 2014-04-14 21:09:14]

R=close

[bradfitz, 2014-04-14 21:09:36]

No problem.



On Mon, Apr 14, 2014 at 2:05 PM, Silvan Jegen <s.jegen@gmail.com> wrote:

> On Mon, Apr 14, 2014 at 01:42:36PM -0700, Brad Fitzpatrick wrote:
> > On Mon, Apr 14, 2014 at 1:35 PM, Silvan Jegen <s.jegen@gmail.com> wrote:
> >
> > > On Mon, Apr 14, 2014 at 12:57:59PM -0700, Brad Fitzpatrick wrote:
> > > > Doesn't seem worth it. Server requests may still be HTTP/1.0 anyway.
> > >
> > > Do you mean server responses? That may be, but the requests will always
> > > be HTTP/1.1 I think:
> > >
> > >
> https://code.google.com/p/go/source/browse/src/pkg/net/http/request.go#48<
> https://code.google.com/p/go/source/browse/src/pkg/net/http/request.go#488
> >
> >
> >
> > No, requests to the server.  The Request struct is used for both outgoing
> > client requests, and incoming server requests.  A non-Go HTTP client can
> > set an "HTTP/1.0" ProtoMajor: 1 ProtoMinor: 0 request to a Go server.
>
> Ah, I see what you mean now.
>
>
> > So the comment you're changing is a valid case, hence my thinking the
> > change is arbitrary.
>
> Having the line
>
> // Client requests always use HTTP/1.1
>
> immediately precede a contradicting comment seemed suspicious to me.
> Maybe it would be better to remove the
>
> // "HTTP/1.0"
> // 1
> // 0
>
> comments since these are the only "annotated" fields anyways.
>
> Thanks for clearing that up and taking the time to respond. Please feel
> free to close the CL. It has been a misunderstanding on my part.
>
>