search

kothar / brotli-go

Not maintained -> see itch.io's more up-to-date fork 👉
https://github.com/itchio/go-brotli
MIT License
82 stars 9 forks source link

Custom dictionaries #40

Closed millisecond closed 7 years ago

millisecond commented 7 years ago

I created a fork with custom dictionary support (https://github.com/crawlcoin/brotli-go) and would like to merge it back upstream, open to a pull request with this support?

kothar commented 7 years ago

Absolutely, please do! The new methods extend the existing API without changing the existing behaviour, is that correct?

My only concern as it stands is that you've modified some of the upstream .c files which makes further updates to the vendored code more difficult:

https://github.com/crawlcoin/brotli-go/commit/edccf2bc3baa7a385e607cfab46e0b69217802b6#diff-1721da9a2c189d371911e516cdd0b5b1

Is this new function part of the upstream changes which have not yet been merged here? If not, could we move it to the header of the decode.go file to avoid future conflicts? https://github.com/crawlcoin/brotli-go/blob/master/dec/decode.go#L11

Thanks for your contribution, looking forward to the PR :) Mike.

millisecond commented 7 years ago

I don't have a background in C/C++ so making those changes was a bit of copy/paste/pray ;) Totally open to better structuring of them. I didn't pull anything more from the upstream just exposed access to functions that didn't have bridging through to the go side.

Will try and move it to the decode.go header when creating the pull-request.

Thanks for the quick reply!

kothar commented 7 years ago

Yep, I know the feeling - there's been plenty of head-scratching over the CGO interfacing and how to get everything to play nicely with go packages.

millisecond commented 7 years ago

Ach, I forgot I had to change all the packaging to get it to function as a fork. Can hand merge the changes back into a new fork while looking to move the header changes, but not sure how I can get a fork to fully run tests with the way go handles the pathing. Any ideas / has anyone else been successful faking out the pathing?

millisecond commented 7 years ago

Nevermind, I had to change pathing to depend on it. Tests are nicely self-contained. Good to go.

kothar commented 7 years ago

Usually I check out my fork in the original package's location in my GOPATH so it just serves as a stand in. Not sure if that helps?

millisecond commented 7 years ago

Hey, I submitted a pull request and kinda forgot about it. Just noticed that the build had failed, but looking at it - the best I can tell is that it's failing because of the indirection through gopkg.in pointing at the released code and not the changed code. Any ideas?