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

Download whole manga #15

Merged
merged 5 commits into from
May 2, 2019
Merged

Conversation

jphager2
Copy link
Contributor

  • WIP NOTE: Only comicextra implemented for now. Will seek feedback
    before continuing.

ISSUE: #14

@jphager2
Copy link
Contributor Author

@Girbons Is this way acceptable for you? Do you have any feedback. Should any concurrency be added?

@Girbons
Copy link
Owner

Girbons commented Apr 29, 2019

Hi @jphager2,

Thanks for the PR, looks good 👍.
I was wondering if we really need the Collection struct, could we collect all the comics just only inside a slice?

Anyway you can always find me on https://gophersinvite.herokuapp.com/

Implement full comic download for all sites
@jphager2 jphager2 changed the title WIP: Download whole manga Download whole manga Apr 30, 2019
@jphager2 jphager2 marked this pull request as ready for review April 30, 2019 06:38
@jphager2
Copy link
Contributor Author

@Girbons, I've also removed MangaHere in this PR. Hope that is okay.

Copy link
Owner

@Girbons Girbons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jphager2 ,

Amazing work on this issue 🎉, I left a couple of comments.

What is needed now is to have a parameter in the command line that allow to download the whole manga.

Once we'll have a solid codebase we are going to handle the concurrency.

pkg/sites/comicextra/comicextra.go Outdated Show resolved Hide resolved
pkg/sites/comicextra/comicextra.go Outdated Show resolved Hide resolved
pkg/sites/mangareader/mangareader.go Outdated Show resolved Hide resolved
pkg/sites/mangatown/mangatown.go Outdated Show resolved Hide resolved
pkg/sites/comicextra/comicextra.go Show resolved Hide resolved
pkg/sites/mangatown/mangatown.go Show resolved Hide resolved
pkg/util/util.go Outdated Show resolved Hide resolved
pkg/sites/mangareader/mangareader.go Outdated Show resolved Hide resolved
pkg/sites/comicextra/comicextra.go Outdated Show resolved Hide resolved
pkg/core/core.go Outdated Show resolved Hide resolved
pkg/sites/loader.go Outdated Show resolved Hide resolved
pkg/sites/loader.go Outdated Show resolved Hide resolved
@Girbons Girbons self-assigned this Apr 30, 2019
@jphager2
Copy link
Contributor Author

Hey @jphager2 ,

Amazing work on this issue , I left a couple of comments.

What is needed now is to have a parameter in the command line that allow to download the whole manga.

Right now it will work that if you provide the url for the chapter, it will only download the chapter, but if you provide the manga/comic page it will download all chapters. So with the flag what should be the behavior if (lets say the flag is --all) the url is https://www.mangareader.net/one-piece and --no-all is passed or the url is https://www.mangareader.net/one-piece/941 and --all is passed?

Once we'll have a solid codebase we are going to handle the concurrency.

Sounds good.

@Girbons
Copy link
Owner

Girbons commented Apr 30, 2019

@jphager2 What I was thinking is that the user may find useful to insert just the chapter url and pass something like --all to download all the chapters. What do you think about it?
--all should be handled when we recognize that the url is https://www.mangareader.net/one-piece/941. Otherwise we'll use the default behaviour.

@jphager2
Copy link
Contributor Author

jphager2 commented May 1, 2019

@jphager2 What I was thinking is that the user may find useful to insert just the chapter url and pass something like --all to download all the chapters. What do you think about it?
--all should be handled when we recognize that the url is https://www.mangareader.net/one-piece/941. Otherwise we'll use the default behaviour.

Yeah makes sense to me.

@Girbons
Copy link
Owner

Girbons commented May 1, 2019

@jphager2 one last thing then I'm going to merge this pr, could you please update the README explaining how to download a whole manga?

@jphager2
Copy link
Contributor Author

jphager2 commented May 1, 2019

@jphager2 one last thing then I'm going to merge this pr, could you please update the README explaining how to download a whole manga?

Sure thing :D

Copy link
Owner

@Girbons Girbons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gonna merge it as soon as possible 👍

@coveralls
Copy link

Pull Request Test Coverage Report for Build 64

  • 132 of 156 (84.62%) changed or added relevant lines in 6 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+8.7%) to 78.053%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/sites/loader.go 36 40 90.0%
pkg/sites/comicextra/comicextra.go 23 28 82.14%
pkg/sites/mangareader/mangareader.go 22 27 81.48%
pkg/sites/mangarock/mangarock.go 25 30 83.33%
pkg/sites/mangatown/mangatown.go 22 27 81.48%
Files with Coverage Reduction New Missed Lines %
pkg/core/core.go 1 67.84%
Totals Coverage Status
Change from base Build 58: 8.7%
Covered Lines: 473
Relevant Lines: 606

💛 - Coveralls

@Girbons Girbons merged commit 03c3e70 into Girbons:master May 2, 2019
@Girbons Girbons mentioned this pull request May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants