Image shows from files inside of epub

[Monirzadeh]

This PR add a method name EmbeddedImages() that download image inside the xhtml than change EPUB to load show from included images instead of URL.
Maybe close #26

[bmaupin]

Very cool!

Contrary to the contribution guidelines, I'm actually wondering if this would be better as a parameter to AddSection? Hmm, but then AddSubSection would need to be modified too...

I tend to think that if it matters which order methods are called in, that can be a bit of a code smell (I just searched and apparently it's called "temporal coupling"). On the other hand, long parameter list is a code smell too 😅

I think if I were to re-create this library, AddSection would take an object as a parameter, which might make adding all of this new functionality a bit easier.

... as you can tell I'm thinking myself in circles here :) @owulveryck do you have any thoughts?

[bmaupin]

After chewing on it for a couple minutes, I'm thinking that having this as a separate method (the way it currently is) is probably the simplest approach. I do think EmbeddedImages should probably be renamed, maybe to just EmbedImages because I think the standard practice is that method names should be verbs.

But I'd still be curious to hear if there are any other thoughts.

[Monirzadeh]

EmbedImages

it change to EmbedImages.
i it good to have that as separate method. (i think most user want all the image Embed not specific section). what do you think? maybe later we can add that as parameter to AddSection too.

[bmaupin]

I suppose it could even be a parameter of the constructor to apply it to the whole Epub. But either way I think it's too much to refactor all of that now, so it makes the most sense to have this as a separate method.

[Monirzadeh]

I suppose it could even be a parameter of the constructor to apply it to the whole Epub. But either way I think it's too much to refactor all of that now, so it makes the most sense to have this as a separate method.

any specific things should be done or it good enough to merge right now?
if something should be done in other PR not bad if create an issue for that to not forgot later.
maybe lots of things can refactor but dive in it yet. (maybe profiling code can give use better view)

[coveralls]

coverage: 87.94% (+1.1%) from 86.799% when pulling 3717254 on Monirzadeh:image-offline into 88f2b0b on bmaupin:main.

[bmaupin]

I merged #66 and rebased your work on top of it just to make sure the tests are passing. Unfortunately this seems to be failing on the Windows runner.

I think it might be related to #19.

Would you mind taking a look?

Thanks!

[Monirzadeh]

@bmaupin
as i test this part of code act differently in windows and Linux but i not found this in windows it not return error if it get 404 but don't have any idea why yet.
why exactly we need to have that in code?

[Monirzadeh]

@bmaupin finaly solve that and all test pass.
beside that i update all unit test to serve file from localhost.
thanks to @fmartingr too.

[bmaupin]

@owulveryck What do you think? I was going to go ahead and merge it, but some changes were made to fetchmedia.go so I thought you might want to take a look first. Feel free to merge it if you think it looks okay, or I can later.

I really like serving the test files from a local file server instead of getting them from the internet, since that should prevent broken URLs from breaking the tests.

As for me the only thing that needs to be changed is the description for EmbedImages needs to be cleaned up a bit since a lot of the duplication from AddImage is unnecessary, but I figured I would just do that myself after merging.

[owulveryck]

I will have a look by the end of this week.

[owulveryck]

LGTM