Skip to content
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

Restructure #24

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Restructure #24

wants to merge 19 commits into from

Conversation

trotha01
Copy link

I restructured a little to make it closer to how gofmt is structured: https://golang.org/src/cmd/gofmt/
Added a doc.go as well

I prefer not having it out the package by default, but it's still a possible flag. Maybe this one is more opinion based.

let me know what you think

@ChimeraCoder
Copy link
Owner

Thanks for restructuring this! I agree that it would be preferable for us to match the way gofmt is laid out.

One thing that we want to be able to do that gofmt does not, however, (and which gofmt probably should) is allow the ability to import the key functionality as a package into a different application (not just this one). There was a pull request recently that added this change, so there's at least one person who has this use case.

Fortunately I think it should be easy for us to adapt your changes this way; I think we just need to move the files that aren't gojson.go into a subdirectory (and update the imports). That way people can just go get this application, but they can also import the functions if they need them.

@ChimeraCoder
Copy link
Owner

Oh, as for outputting the flag by default, the idea is that it makes it easier to use gojson from go generate.

Is there a reason that omitting the package is preferable for your workflow?

@trotha01
Copy link
Author

That's a good point. I updated the package name so it's importable.
Here's a sample I used to test: http://play.golang.org/p/h4KTBGQqc8

I haven't used go generate. Do you have any examples I can play with?
The reason I took it out, is so I can use it inline. I can pipe the output to an already existing go file, or with vim, I can change json in the file directly.
I'll have to look into go generate to see what it does.

@trotha01
Copy link
Author

I removed the change to suppress outputting package by default, so it's back to the original functionality.

I also couldn't think of a nice way to structure this so that it's both a library for people to import, and an executable to use, so I made a post in golang-nuts to see what other people would think.
https://groups.google.com/forum/#!topic/golang-nuts/jSbOE2UJxKU

@trotha01
Copy link
Author

okay, updated per Matt's comment in the golang-nuts group (link in previous comment)

Let me know what you think

@trotha01
Copy link
Author

ping?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants