New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup package synopsis. #41

Merged
merged 16 commits into from Aug 18, 2014

Conversation

Projects
None yet
3 participants
@slimsag
Member

slimsag commented Aug 16, 2014

Here I've modified BlankLineStrippingWriter to take in a start offset, a literal number of lines to write without stripping blank lines.

Then we hard-code the offsets found in the template files to the Go package statement (17/3/3).

This allows us to have cleaner package synopsis:

This fixes #39.

@capnm capnm referenced this pull request Aug 16, 2014

Closed

ugly package synopsis #39

// Package {{.Name}} implements Go bindings to OpenGL.
//
// This package was automatically generated using Glow:
// http://github.com/go-gl/glow

This comment has been minimized.

@errcw

errcw Aug 16, 2014

Member

tiny nit: prefer a two-space indent (here and below)

@errcw

errcw Aug 16, 2014

Member

tiny nit: prefer a two-space indent (here and below)

This comment has been minimized.

@slimsag

slimsag Aug 17, 2014

Member

two-space indent? what do you mean?

@slimsag

slimsag Aug 17, 2014

Member

two-space indent? what do you mean?

This comment has been minimized.

@errcw

errcw Aug 17, 2014

Member

I meant to indent the URL and SVN revision at two spaces from the top line (three from the comment). It's a really trivial style comment.

@errcw

errcw Aug 17, 2014

Member

I meant to indent the URL and SVN revision at two spaces from the top line (three from the comment). It's a really trivial style comment.

@@ -54,7 +61,7 @@ func isBlank(line string) bool {
// Write appends the contents of p to the BlankLineStrippingWriter.
// The return values are the length of p and the error of the underlaying io.Writer.
func (w BlankLineStrippingWriter) Write(p []byte) (int, error) {
func (w *BlankLineStrippingWriter) Write(p []byte) (int, error) {

This comment has been minimized.

@errcw

errcw Aug 16, 2014

Member

We should also update the tests for this functionality. (And I think the tests might need a clean-up pass?)

@errcw

errcw Aug 16, 2014

Member

We should also update the tests for this functionality. (And I think the tests might need a clean-up pass?)

@errcw

View changes

Show outdated Hide outdated package.go Outdated
@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Aug 17, 2014

Member

PTAL @errcw and @capnm .

  • Merged the changes from capnm (PR #44)
  • Indented the synopsis comments with two spaces (looks identical to original picture).
  • Fixed tests still trying to use BlankLineStrippingWriter that capnm removed.
  • Ran gofmt on package.go
  • Fixed relative path usage in Makefile (./glow vs just glow).
  • Regenerate wrappers using the Makefile (and formatted using goimports/gofmt command).
Member

slimsag commented Aug 17, 2014

PTAL @errcw and @capnm .

  • Merged the changes from capnm (PR #44)
  • Indented the synopsis comments with two spaces (looks identical to original picture).
  • Fixed tests still trying to use BlankLineStrippingWriter that capnm removed.
  • Ran gofmt on package.go
  • Fixed relative path usage in Makefile (./glow vs just glow).
  • Regenerate wrappers using the Makefile (and formatted using goimports/gofmt command).
@errcw

This comment has been minimized.

Show comment
Hide comment
@errcw

errcw Aug 17, 2014

Member

Most of the secondary changes look good (I'm always a huge fan of continuous refactoring and clean-up) but I'm really hesitant to alter package.tmpl to introduce all those comments. I fear it makes the template significantly more difficult to read and maintain. I readily agree that BlankLineStrippingWriter is a strange construct but I'd prefer to bear the burden in utility code than the core template.

Member

errcw commented Aug 17, 2014

Most of the secondary changes look good (I'm always a huge fan of continuous refactoring and clean-up) but I'm really hesitant to alter package.tmpl to introduce all those comments. I fear it makes the template significantly more difficult to read and maintain. I readily agree that BlankLineStrippingWriter is a strange construct but I'd prefer to bear the burden in utility code than the core template.

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Aug 17, 2014

Member

Okay. Lets try once more, with a different approach. Comment-based annotations will be accepted to define sections of code that should have their blank lines kept in-tact, like so:

//
//glow:keepspace
//
// Hello World!

//
//glow:rmspace
//

Then the blank line stripper would produce output like so:

//
// Hello World!

//

I used //glow:command because Go also uses annotation comments like that for various things. I like this method because it allows for annotations to also be generated elsewhere by the template (if we wanted better spacing between functions or something later on)

  • Reverted changes by @capnm (adding back BlankLineStrippingWriter and it's tests).
  • Add comment-based glow:keepspace and glow:rmspace annotations (and tests for it).
  • Modify templates to use the new annotations and get nice GoDoc synopsis (fixes issue #39).
  • Regenerate and format generated wrappers with goimports -w -l .
Member

slimsag commented Aug 17, 2014

Okay. Lets try once more, with a different approach. Comment-based annotations will be accepted to define sections of code that should have their blank lines kept in-tact, like so:

//
//glow:keepspace
//
// Hello World!

//
//glow:rmspace
//

Then the blank line stripper would produce output like so:

//
// Hello World!

//

I used //glow:command because Go also uses annotation comments like that for various things. I like this method because it allows for annotations to also be generated elsewhere by the template (if we wanted better spacing between functions or something later on)

  • Reverted changes by @capnm (adding back BlankLineStrippingWriter and it's tests).
  • Add comment-based glow:keepspace and glow:rmspace annotations (and tests for it).
  • Modify templates to use the new annotations and get nice GoDoc synopsis (fixes issue #39).
  • Regenerate and format generated wrappers with goimports -w -l .
@capnm

This comment has been minimized.

Show comment
Hide comment
@capnm

capnm Aug 17, 2014

Contributor

I like the complete control of the whitespace inside the template more than a better readability. But I always used the Go templates this way, so maybe I don't feel the readability as bad as the others.

@slimsag this approach looks much better to me.

Contributor

capnm commented Aug 17, 2014

I like the complete control of the whitespace inside the template more than a better readability. But I always used the Go templates this way, so maybe I don't feel the readability as bad as the others.

@slimsag this approach looks much better to me.

@errcw

View changes

Show outdated Hide outdated package.go Outdated
@errcw

View changes

Show outdated Hide outdated util.go Outdated
@@ -11241,9 +11248,10 @@ package gl
import "C"
import (
"errors"
"unsafe"

This comment has been minimized.

@errcw

errcw Aug 17, 2014

Member

How did we end up with new blank lines here? Shouldn't the writer have stripped them out?

@errcw

errcw Aug 17, 2014

Member

How did we end up with new blank lines here? Shouldn't the writer have stripped them out?

This comment has been minimized.

@slimsag

slimsag Aug 17, 2014

Member

goimports (http://godoc.org/code.google.com/p/go.tools/cmd/goimports) is a command in the go.tools repo and formats imports in a standard way -- it places a blank line between std library packages and thirdparty ones.

If you want, I can use gofmt instead and we can drop the blank line.

@slimsag

slimsag Aug 17, 2014

Member

goimports (http://godoc.org/code.google.com/p/go.tools/cmd/goimports) is a command in the go.tools repo and formats imports in a standard way -- it places a blank line between std library packages and thirdparty ones.

If you want, I can use gofmt instead and we can drop the blank line.

This comment has been minimized.

@errcw

errcw Aug 18, 2014

Member

My only concern is that these changes will be reverted the next time we update the generated files. It's fine for now.

@errcw

errcw Aug 18, 2014

Member

My only concern is that these changes will be reverted the next time we update the generated files. It's fine for now.

@errcw

This comment has been minimized.

Show comment
Hide comment
@errcw

errcw Aug 17, 2014

Member

This latest version looks great. I only have a few very minor comments and I'll be happy to merge this in.

Member

errcw commented Aug 17, 2014

This latest version looks great. I only have a few very minor comments and I'll be happy to merge this in.

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Aug 17, 2014

Member

PTAL

Member

slimsag commented Aug 17, 2014

PTAL

errcw added a commit that referenced this pull request Aug 18, 2014

@errcw errcw merged commit 9eef101 into go-gl:master Aug 18, 2014

@slimsag slimsag deleted the slimsag:clean_synopsis branch Aug 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment