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

Add Ninemanga source #392

Merged
merged 3 commits into from
Jul 23, 2023
Merged

Add Ninemanga source #392

merged 3 commits into from
Jul 23, 2023

Conversation

feserr
Copy link
Contributor

@feserr feserr commented Jul 11, 2023

Checklist:

  • Updated source's version for individual source changes
  • Updated all sources' versions for template changes
  • Set appropriate nsfw value
  • Did not change id even if a source's name or language were changed
  • Tested the modifications by running it on the simulator or a test device

Closes #43
Closes #158

@feserr
Copy link
Contributor Author

feserr commented Jul 12, 2023

@Skittyblock Could you review it?

Copy link
Owner

@Skittyblock Skittyblock left a comment

Choose a reason for hiding this comment

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

  • add newlines at the end of jsons and use tabs instead of spaces for indentation
  • run cargo +nightly fmt
  • fix clippy warnings generated with cargo +nightly clippy

Copy link
Owner

Choose a reason for hiding this comment

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

Why do you have this? You're not using it anywhere and there's only a single value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy and paste from another source 😅 . I will remove it

Copy link
Owner

Choose a reason for hiding this comment

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

It's still here

NineMangaSource {
base_url,
language,
completed_series: "Completed Series",
Copy link
Owner

Choose a reason for hiding this comment

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

Why are you setting completed_series the same as the default?

Comment on lines 82 to 89
author: String::new(),
artist: String::new(),
description: String::new(),
url: String::new(),
categories: Vec::new(),
status: MangaStatus::Unknown,
nsfw: MangaContentRating::Safe,
viewer: MangaViewer::Default,
Copy link
Owner

Choose a reason for hiding this comment

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

use ..Default::default()

cover,
title,
author,
artist: String::new(),
Copy link
Owner

Choose a reason for hiding this comment

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

..Default::Default()

Comment on lines 151 to 157
if nsfw_genres.contains(&category.clone().as_str()) {
nsfw = MangaContentRating::Nsfw;
}
if category.clone().as_str() == "Webtoon" {
viewer = MangaViewer::Scroll;
}
categories.push(category.clone());
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think you need to clone this string three times

let status = status_from_string(html.select(".bookintro .red").first().text().read());
let mut categories = Vec::new();
let mut nsfw = MangaContentRating::Safe;
let mut viewer = MangaViewer::Default;
Copy link
Owner

Choose a reason for hiding this comment

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

don't use Default (it's deprecated), use Rtl

Comment on lines 213 to 217
title: String::new(),
volume: -1.0,
chapter: chapter_number,
date_updated,
scanlator: String::new(),
Copy link
Owner

Choose a reason for hiding this comment

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

..Default::default()

for manga in html.select(".direlist dl").array() {
let manga_node = manga.as_node();
let title = manga_node.select("a.bookname").text().read();
let id = manga_node.select("a.bookname").attr("href").read();
Copy link
Owner

Choose a reason for hiding this comment

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

Do not set a full url as a manga id. Use the path and prepend the base url when you actually need the full url. Using the url as an id is bad practice since it will break if the website changes domains.

Comment on lines 245 to 275
base64: String::new(),
text: String::new(),
Copy link
Owner

Choose a reason for hiding this comment

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

..Default::default()

Copy link
Owner

Choose a reason for hiding this comment

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

still not addressed

@feserr
Copy link
Contributor Author

feserr commented Jul 13, 2023

@Skittyblock I have update the branch addressing all your changes/comments. I tested and it is working Ok using the ID instead of the full URL.

title,
author,
description,
url: id,
Copy link
Owner

Choose a reason for hiding this comment

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

since you changed id, it shouldn't be an actual url

Copy link
Owner

Choose a reason for hiding this comment

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

don't include .vscode folder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I stagged unintentionally 😅


chapters.push(Chapter {
id: chapter_id,
volume: -1.0,
Copy link
Owner

Choose a reason for hiding this comment

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

remove the volume: -1.0 since it's include in the defaults

Suggested change
volume: -1.0,

Comment on lines 245 to 275
base64: String::new(),
text: String::new(),
Copy link
Owner

Choose a reason for hiding this comment

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

still not addressed


pub fn handle_url(&self, url: String) -> Result<DeepLink> {
Ok(DeepLink {
manga: Some(Self::parse_manga_details(self, url)?),
Copy link
Owner

Choose a reason for hiding this comment

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

you changed id so it shouldn't be a url any longer, you need to parse the id out of it

Copy link
Owner

Choose a reason for hiding this comment

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

still not addressed

@feserr feserr force-pushed the ninemanga branch 2 times, most recently from f7ce703 to 5988e6b Compare July 16, 2023 13:56
Copy link
Owner

Choose a reason for hiding this comment

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

this is still here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I forgot to remove it.

.read()
.replace(".html", "-10-1.html");

let url = chapter_id.clone();
Copy link
Owner

Choose a reason for hiding this comment

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

chapter id should be the relative url, not the full url, just like manga ids

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

pages
}

pub fn get_search_url(
Copy link
Owner

Choose a reason for hiding this comment

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

for future reference, it's better to use QueryParameters from the aidoku-rs helpers crate

@Skittyblock Skittyblock merged commit 5717532 into Skittyblock:main Jul 23, 2023
2 checks passed
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.

NineManga
3 participants