Skittyblock / aidoku-community-sources

Public sources for the Aidoku application
Apache License 2.0
418 stars 144 forks source link

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

Closed hanatsumi closed 4 months ago

hanatsumi commented 4 months ago

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:

Skittyblock commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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