-
Notifications
You must be signed in to change notification settings - Fork 39
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
Account for decimals in unread chapter count #335
Conversation
This removes reliance on Chapter.chapterNumber (which is nullable)
So the current logic you see is mainly for 3 reasons:
This is why I chose to work with
I think it's worth addressing those points, but it's not trivial to consider all of these (and wouldn't be as simple as either of our implementations). Let me know if you disagree! |
That makes a lot more sense now with context 👍🏾 I agree with all your considerations - sounds like a tricky situation whichever way you look at it. When I get a chance (most likely this weekend) I'll have a go at updating the implementation. |
This should be ready for a re-review. I've tried my best to follow all the considerations but with there being so many it isn't perfect. The code has also become a lot more complicated. Trade paperback comics on ReadComicOnline will show the unread count inverted (basically the read count) which is completely wrong. This is because there are no chapter numbers present so we can't exactly sort it to get the absolute chapter number. I'm planning on opening a PR on Tiyo that'll hopefully address this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left minor comments but I'm OK with merging. Thank you!
const gap = Math.ceil(chapterNumber - previousChapNumber) - 1; | ||
if (gap > 1) { | ||
// A gap between chapters was found. Account for this in the absolute numbers | ||
absoluteNumber += gap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you could avoid needing to increment both here and only set absoluteNumber
after this block (since it's not used yet)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh yes, didn't catch that
let chapter = groupedChapters.find((chap) => chap.read); | ||
if (chapter === undefined) { | ||
[chapter] = groupedChapters; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The [chapter]
assignment here looks pretty weird, it seems to work but I don't really understand why. I think this section is equivalent to just:
let chapter = groupedChapters.find(chap => chap.read) || groupedChapters[0];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I did this was because the linter kept complaining at me, so I assumed it was because of the project level linter config 😅
In hindsight it was probably just my VSCode extension 🤷🏾
Fixes #298
I was hesitating on whether I should open a PR for this as I think the behaviour is intentional (however I don't really agree with the current implementation).
I tracked it down to the
markChapter
method which at first seemed like it was over engineering something really simple, but I think the original dev who wrote this intended for chapters with decimal points to be consolidated as one chapter (e.g 41.1, 41.2, 41.3, 41.4 is just chapter 41). I think this is a pretty odd way of looking at it and is very confusing at first glance - it took me a second to figure out why this was implemented the way that it is. As it relies heavily on chapter numbers being present, it means that any chapter with no chapter number is completely ignored (as seen in the Moon Knight screenshot in the original issue).Feel free to close this PR if you think the current implementation is correct.