go-comic-converter icon indicating copy to clipboard operation
go-comic-converter copied to clipboard

Expose the internal/epub package

Open pinheirolucas opened this issue 1 year ago • 11 comments

Hi @celogeek.

I want to integrate go-comic-converter with one of my projects.

I've looked at the codebase and figured out that the core epub conversion logic is nicely decoupled from the CLI and configuration logic on a github.com/celogeek/go-comic-converter/internal/epub package.

The problem is that this package lies on the internal package. As stated on the feature release notes, in this case, no packages outside of the root github.com/celogeek/go-comic-converter would be able to import what's within the internal package.

I don't know what's the design decision behind doing that, but it would be good to move the github.com/celogeek/go-comic-converter/internal/epub package to something like github.com/celogeek/go-comic-converter/pkg/epub.

Although I could integrate with it using something like os/exec, by exposing the package, go-comic-converter could also integrate at the code level with other Go packages, which is way better.

A good example of what I'm trying to describe here is github.com/golang-migrate/migrate, where the core package has a CLI but also supports being embedded by other packages.

Are there any concerns about doing that? I can spin up a quick PR with it if you wish, as I'll be forking the repo to implement this change anyway.

pinheirolucas avatar Jan 03 '24 16:01 pinheirolucas

Hi, I usually put my code in internal, as I move it a lot, and keep only the exposed part outside. in that case the tools that will handle properly the call to ePub package with all the available options.

I have severals sub package that could be shared by putting it outside the internal. The only issue is when I start doing it, I need to take care to avoid doing breaking changes or force a major update.

I will see if I can share the ePub api properly, without the risk of breaking stuff later. Keeping it clean.

Working on it.

celogeek avatar Jan 04 '24 14:01 celogeek

I've start a branch. I have a lot of subpackage, which I change a bit the struct Can you take a look and let me know ? I will keep working to improve the result.

celogeek avatar Jan 04 '24 18:01 celogeek

Sure. I'll dedicate some time to take a look at the diff tomorrow. Thanks!

pinheirolucas avatar Jan 05 '24 02:01 pinheirolucas

@celogeek I've taken a look at the code, and have a couple of suggestions on the API design of the exposed packages.

Could you create a draft PR? So we can move this discussion there and I can point out my suggestions.

pinheirolucas avatar Jan 09 '24 20:01 pinheirolucas

here: https://github.com/celogeek/go-comic-converter/pull/27

I was thinking of creating a real interface for epub, with everything to plug hooks like progress, and everything.

celogeek avatar Jan 10 '24 08:01 celogeek

Sure. I create a pr but after a while I close it. I haven't got time to work on it.

Please do. I will check.

celogeek avatar Sep 24 '24 18:09 celogeek

Sorry, I deleted my previous message by accident ... I will do a PR soon, from my understanding, only moving the packages out of the pkg/external do the trick but I could be wrong (everything seems to work fine but I lack tests to validate that)

thomsmoreau avatar Sep 24 '24 20:09 thomsmoreau

PR link: #40 As I said it is pretty basic and I just moved the folder pkg out of the internal folder

thomsmoreau avatar Sep 24 '24 20:09 thomsmoreau

Well. Internals may move a lot while public api remain stable. Exposing everything this way, will make any changes harder, as we want to keep the interface.

I was thinking of a configurable api that can be used by external client or a command line provided like the one we have.

Like New().WithOption(...) Or New(option1, option2...) Like New(WithTitle('test'), WithProfile('ks'),OnProgress(func(p){...}).Write('dest')

celogeek avatar Sep 24 '24 22:09 celogeek

I understand you concerns about making everything public, I will try to have a deeper understanding of how things work in the repo, do you have any idea of what you would like to expose ?

If I had to choose from the options provided maybe New(option1, option2...) is better since we could create a global Options struct composed of multiple options struct parsed using a yaml or json file or with the flags provided which could give the opportunity to have maybe some presets for different scenarios

thomsmoreau avatar Sep 25 '24 12:09 thomsmoreau

I export epub and epub options in a public way here an example of usage

package main

import (
	"github.com/celogeek/go-comic-converter/v3/pkg/epub"
	"github.com/celogeek/go-comic-converter/v3/pkg/epuboptions"
)

func main() {

	err := epub.New(epuboptions.EPUBOptions{
		Input:  "mymanga.cbz",
		Output: "mymanga.epub",
		Image: epuboptions.Image{
			Quality:  90,
			Manga:    true,
			HasCover: true,
			View: epuboptions.View{
				Width:       1200,
				Height:      1920,
				AspectRatio: 1.6,
			},
			GrayScale: true,
			Resize:    true,
			Format:    "jpeg",
		},
		Workers: 8,
	}).Write()
	if err != nil {
		return
	}
}

The next step is to change the progress to make it configurable. Like:

  • OnProcess(current, total)
  • OnWrite()
  • Done()

celogeek avatar Jan 05 '25 16:01 celogeek