Skip to content

Fix shift-endnotes to get --decrement working and handle non-contiguous note ids #819

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

Merged
merged 1 commit into from
Apr 14, 2025

Conversation

vr8hub
Copy link
Contributor

@vr8hub vr8hub commented Apr 12, 2025

In adding tests for shift-endnotes I discovered --decrement didn't work. And upon investigating that, I found --increment didn't either if the note ids weren't contiguous.

The way that the class shift_endnotes function worked, it create a range() for the notes to be modified, but 1) that range was wrong if the ids weren't perfectly contiguous, and 2) was, as far as I can tell, wrong for decrements all the time.

For both ranges, it assumed the count of endnotes equaled the highest endnote#, and that's definitely not true once a shift has happened, and it might not be true before a shift has happened, if the reason for doing a shift is to reduce a gap caused by the deletion of an endnote in the middle. It gets worse if a set of notes are shifted, then a subset of those same notes are shifted again. This can easily happen if someone needs to add a note at note-10 and another at note-27, e.g.

Conceptually, what needs to happen with shift is that starting at whatever endnote# is specified, all the remaining notes, whatever their numbers, need to be shifted by the amount, either up or down. So that's what I did—I made a list of all the integer endnote numbers1, removed the numbers that are less than the starting (target) endnote number, and then bump the remaining ids by the appropriate amount.

Something that the command does not take into account was whether a shift can produce duplicate endnote ids. I did not change it to do so, either.

1I believe we have one or two books with non-integer endnotes; this command won't work on them, before or after these changes, since the starting # is specified as an integer and the code looks specifically for note-{number}.

EDIT:
I tested on Don Quixote and Pepys: the former took just under 8 seconds, the latter took 1:49. I didn't take the time to try Gibbon. :)

EDIT 2:
Sorry, forgot—shift-illustrations appears to have the same problem. I'll fix it once this is resolved and merged.

@vr8hub
Copy link
Contributor Author

vr8hub commented Apr 12, 2025

What mypy is complaining about isn't an error, as far as I'm concerned. reversed(list) produces an iterable, which is handled the same as a list. Working around that means we have to make deep copies of the list, which is expensive and, IMO, unnecessary. Iterating over a list and over an iterable are done exactly the same way, as is done here. Let me know what you would like done here.

@vr8hub vr8hub force-pushed the fix_shift-endnotes branch from 91c5bf9 to f7f2933 Compare April 12, 2025 22:40
@vr8hub
Copy link
Contributor Author

vr8hub commented Apr 12, 2025

I did some playing around with timings, and using a negative slice to create the reverse list is just as fast (no changes to times for Quixote or Pepys) and shuts mypy up (locally, at least).

@vr8hub vr8hub force-pushed the fix_shift-endnotes branch from f7f2933 to 87676ca Compare April 12, 2025 22:48
@acabal acabal merged commit 93acdc9 into standardebooks:master Apr 14, 2025
1 check passed
@acabal
Copy link
Member

acabal commented Apr 14, 2025

Great work, thanks! If you can sort out shift-illustrations soon then I'll wait to release the next version of the tools until then.

@vr8hub
Copy link
Contributor Author

vr8hub commented Apr 14, 2025

Yes, I have the fix and the tests done, just waiting for this. It will be tonight or tomorrow night, though, this is busy week during the day.

@vr8hub vr8hub deleted the fix_shift-endnotes branch April 14, 2025 22:56
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.

2 participants