search

akavel / rsrc

Tool for embedding .ico & manifest resources in Go programs for Windows.
MIT License
1.22k stars 122 forks source link

Moved bin logic to specific package to export the internal logic #15

Closed asticode closed 7 years ago

asticode commented 7 years ago

I need to use this repo logic in a GO project so instead of executing the binary I think this would be best if the internal logic was exported.

What do you think?

asticode commented 7 years ago

All done here :)

asticode commented 7 years ago

@akavel what do you think about @shurcooL 's proposition?

akavel commented 7 years ago

If you don't have time for more work on this, please let me know and I'll merge as is, I'll create issues and leave it for future users or future me to fix the bugs (which are bad but not world-shattering).

asticode commented 7 years ago

@akavel I've made the requested changes

No worries regarding the time I can work on this, as long as it doesn't become an inefficient ping-pong of changes which it is not :D

akavel commented 7 years ago

Ah, sorry: please also add yourself to AUTHORS file; TIA. (Optionally also upgrade the final copyright date in README and in --help message.)

asticode commented 7 years ago

@akavel from experience, the CI travis integration doesn't work until there's a .travis.yml at the root of the project, therefore I've rebased on the new tip. It seems to be working (at least I see the Some checks haven’t completed yet in Github's interface).

I've made the changes.

However I've a last question regarding moving this code as a library: line 53 and 97 of rsrc/rsrc.go, there's a fmt.Println that logs some information. That may not be desired by someone using this library. To your opinion is this something that should be removed or updated?

akavel commented 7 years ago

Thanks! Hm; about the Printlns, they have some informative value; but now that I think of it they can probably be deduced afterwards. Then if you could please comment them out, and above one/all of them add a:

// TODO(akavel): reintroduce the Printlns in package main after Embed returns

Thanks.

edit: or optionally do the Printlns in main already if you fancy! :)

Please note I probably won't be available to respond from today afternoon till Sunday.

asticode commented 7 years ago

@akavel commented the Printlns

akavel commented 7 years ago

Thanks!