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

use bindata and package assets #10

Closed
wants to merge 1 commit into from
Closed

use bindata and package assets #10

wants to merge 1 commit into from

Conversation

djcas9
Copy link

@djcas9 djcas9 commented May 13, 2015

So, this is what I would do so that no external deps are needed outside of the import. This will solve the GOPATH issues and also remove external files making a single binary build easy.

@djcas9
Copy link
Author

djcas9 commented May 13, 2015

@albrow Let me know your thoughts on this. This would just require rebuilding the bindata.go fille when any lua scripts or changed, added or removed. go-bindata scripts/... in the root dir. (https://github.com/jteeuwen/go-bindata)

@albrow
Copy link
Owner

albrow commented May 13, 2015

@mephux, thank for looking into this and acting so quickly. I was actually just checking out binddata and it looks pretty cool! As it turns out, I simultaneously worked on my own solution and created PR #11. I'm going to take a little bit of time to look over both solutions and figure out which will work best, and I'd love to hear your feedback on #11 too.

At first glance, it appears that the binddata solution adds a little more complexity and overhead. Correct me if I'm wrong, but I believe Zoom would still need to read the lua "files" from the binddata library at runtime and then parse them with a call to redis.NewScript.

In comparison, #11 just pulls the content out of the lua files and passes them as strings into redis.NewScript. I personally find the generated code easier to understand (even if the code that generates it at scripts/main.go is a little messy for now).

I would appreciate it if you head over to #11 and verify that it does work for you, i.e. that the tests still pass with your setup. In the meantime, I will be verifying that the tests pass for my setup with #10.

@albrow
Copy link
Owner

albrow commented May 13, 2015

Also please note for next time... as described in the contributing section of the README, I would prefer that pull requests are merged into develop, not master. This helps me manage the branching model and version numbers in a sane way.

I think I'm going to move the instructions into a Contributing.md file to make it easier for people to find.

@djcas9
Copy link
Author

djcas9 commented May 13, 2015

@albrow no problem. So the bindata is loaded into memory. So, if I import your lib into my CLI application it just loads that into memory and reads from that. So, it's pretty much the same as reading it in as a string. The plus side of bindata is not having to ship the scripts folder with my binary or program.

@albrow
Copy link
Owner

albrow commented May 13, 2015

@mephux okay I see. I've looked at the output at bindata.go and run go-bindata scripts/... myself. As expected, tests still pass for my setup (a pretty standard single GOPATH and no dependency manager). Also passes if you run from any directory, which is good.

There's still a tad more overhead in terms of function calls, converting things from bytes, etc. But I think performance-wise it's negligible. I'm more concerned about the complexity in terms of understanding the code. The thing to consider now is whether I want to import a third-party library or just use the code I wrote. They both have the same effect and the same features. Pros and cons to each. I just need a moment to think about it.

@djcas9
Copy link
Author

djcas9 commented May 13, 2015

@albrow yea, you're solution looks fine - going to close this.

@djcas9 djcas9 closed this May 13, 2015
@albrow
Copy link
Owner

albrow commented May 13, 2015

Okay. Thanks for bringing this issue up! It's now fixed in version 0.9.3 on master. Please don't hesitate to create more PRs or issues in the future :)

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.

None yet

2 participants