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

Expose the internal/epub package #26

Open
pinheirolucas opened this issue Jan 3, 2024 · 10 comments
Open

Expose the internal/epub package #26

pinheirolucas opened this issue Jan 3, 2024 · 10 comments
Labels
enhancement New feature or request in progress I'm working on it

Comments

@pinheirolucas
Copy link

pinheirolucas commented Jan 3, 2024

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.

@celogeek
Copy link
Owner

celogeek commented Jan 4, 2024

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
Copy link
Owner

celogeek commented Jan 4, 2024

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.

@pinheirolucas
Copy link
Author

pinheirolucas commented Jan 5, 2024

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

@pinheirolucas
Copy link
Author

pinheirolucas commented Jan 9, 2024

@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 pinheirolucas changed the title Expose the github.com/celogeek/go-comic-converter/internal/epub package Expose the internal/epub package Jan 9, 2024
@celogeek celogeek linked a pull request Jan 10, 2024 that will close this issue
@celogeek
Copy link
Owner

here: #27

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

@celogeek
Copy link
Owner

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.

@thomsmoreau
Copy link

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
Copy link

thomsmoreau commented Sep 24, 2024

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

@celogeek
Copy link
Owner

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')

@thomsmoreau
Copy link

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

@celogeek celogeek added the enhancement New feature or request label Oct 4, 2024
@celogeek celogeek added the in progress I'm working on it label Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in progress I'm working on it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants