goccmack / gocc

Parser / Scanner Generator
Other
622 stars 48 forks source link

added make regenerate and regenerate all examples and tests #33

Closed awalterschulze closed 8 years ago

mewmew commented 8 years ago

Looks good.

Would you mind adding a goimports invocation from the regenerate rule?

The current diff changes the import order.

 import (

    // "fmt"
    // "github.com/goccmack/gocc/example/astx/util"

+   "github.com/goccmack/gocc/example/astx/token"
    "io/ioutil"
    "unicode/utf8"
-
-   "github.com/goccmack/gocc/example/astx/token"
 )
awalterschulze commented 8 years ago

I have added goimports and also changed the templates to do this right in the first place.

awalterschulze commented 8 years ago

On second thought, I have removed goimports, since this introduces a new dependency and we don't currently have any dependencies in this project.

awalterschulze commented 8 years ago

But the generated code is now goimports compatible.

mewmew commented 8 years ago

I have added goimports and also changed the templates to do this right in the first place.

Great!

But the generated code is now goimports compatible.

Lovely.

I use goimports along with golint and other tools to validate this on every commit using Travis CI. While it does introduce a Travis dependency, it also provides a sanity check of the code. But then again, I guess the same could be argued for adding gographviz test cases as a Travis dependency.

mewmew commented 8 years ago

You have a :+1: from me to merge this.

awalterschulze commented 8 years ago

Yes I am ok with adding goimports etc as part of the travis build, but not as part of a plain regenerate

On Sat, 3 Dec 2016, 19:05 Robin Eklind, notifications@github.com wrote:

You have a 👍 from me to merge this.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/goccmack/gocc/pull/33#issuecomment-264655174, or mute the thread https://github.com/notifications/unsubscribe-auth/ABvsLT5ZsgfS_cCbdJVnWO-Sktd6EP52ks5rEa97gaJpZM4LCyLT .

mewmew commented 8 years ago

Yes I am ok with adding goimports etc as part of the travis build, but not as part of a plain regenerate

Oh, of course. Sorry for the confusion. This was my intention all along.

mewmew commented 8 years ago

I'm curious if we should discuss the review process of Gocc. Should we require two or more collaborators to +1 a PR before merging it, or is one enough if not otherwise specified by CC:ing specific people?

awalterschulze commented 8 years ago

I prefer to at least have a plus one from one of the two of you and no objections, before I merge code I changed. I am also happy when you merge something, someone else is trying to contribute. But thats my opinion. @goccmack what do you think?

mewmew commented 8 years ago

Sounds good to me. Thanks for merging this! Already in use at llir/llvm : )