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

fix(mangadex): do not fail chapter parsing if externalUrl is null #661

Merged
merged 1 commit into from
May 25, 2024

Conversation

hanatsumi
Copy link
Contributor

@hanatsumi hanatsumi commented May 11, 2024

Looking at #25, it looks like the idea was to filter out chapters where the externalUrl is not null; however the current logic filters both when externalUrl is null and when it's a string (so essentially always I think?). Changed the logic to filter only chapters where externalUrl is a string.

A manga where chapters are filtered incorrectly is Houseki no Kuni (API call, note how externalUrl is always null).

I haven't tested this with Aidoku itself (as I do not own a iOS/macOS device), but with a custom client that uses Aidoku's sources; it seems to work properly

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

@Skittyblock
Copy link
Owner

Honestly, looking at the code, it seems like you're right, but in practice these chapters aren't actually being filtered out. If you look at Houseki no Kuni in Aidoku, all the proper chapters are there. @beerpiss do you know why the code is checking if the fetched attribute is none?

@hanatsumi
Copy link
Contributor Author

Yeah, it's a bit weird to me that this works within Aidoku, because by looking at ValueRef's implementation it seems like either the check for a ValueKind::Null (inside is_none()) or for a ValueKind::String (inside as_string()) will succeed; so I'd assume ext_url is some other ValueKind here?

@beer-psi
Copy link
Collaborator

Honestly, looking at the code, it seems like you're right, but in practice these chapters aren't actually being filtered out. If you look at Houseki no Kuni in Aidoku, all the proper chapters are there. @beerpiss do you know why the code is checking if the fetched attribute is none?

It's been a super long time that I couldn't remember, sorry. Maybe I copied it from the Tachi extension?

@Skittyblock
Copy link
Owner

@hanatsumi do you know any titles that actually come out differently with the change? I think I'm probably fine with just merging it but I'm curious if there's an actual difference

@hanatsumi
Copy link
Contributor Author

@Skittyblock On my custom client, pretty much all titles change, because as it is now chapter parsing always fails due to the is_none check. I don't really know if any titles change within Aidoku, but I believe the behaviour should stay the same.

@Skittyblock
Copy link
Owner

Alright, likely an issue with Aidoku's api or aidoku-rs then.

@Skittyblock Skittyblock merged commit b528499 into Skittyblock:main May 25, 2024
1 of 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.

3 participants