MAINT: Consistent terminology for outline items

[mtd91429]

This PR makes sure PyPDF2 uses a consistent nomenclature for the outline:

• Outline: A document has exactly one outline (also called "table of contents", in short toc). That outline might be empty.
• Outline Item: An element within an outline. This is also called a "bookmark" by some PDF viewers.

Concretely, this means that some names will be deprecated to ensure consistency:

PdfReader

• outlines ➔ outline
• _build_outline() ➔ _build_outline_item()

PdfWriter

• Keep get_outline_root()
• add_bookmark_dict() ➔ add_outline()
• add_bookmark() ➔ add_outline_item()

PdfMerger

• _write_bookmarks() ➔ _write_outline()
• _write_bookmark_on_page() ➔ _write_outline_item_on_page()
• _associate_bookmarks_to_pages() ➔ _associate_outline_items_to_pages()
• find_bookmark() ➔ find_outline_item()
• Keep _trim_outline()

generic.py

• Bookmark ➔ OutlineItem

Closes #1098

[codecov]

Codecov Report

Merging #1156 (b78fccd) into main (844f238) will decrease coverage by 0.15%.
The diff coverage is 88.96%.

@@            Coverage Diff             @@
##             main    #1156      +/-   ##
==========================================
- Coverage   92.24%   92.08%   -0.16%     
==========================================
  Files          24       24              
  Lines        4811     4866      +55     
  Branches      991      996       +5     
==========================================
+ Hits         4438     4481      +43     
- Misses        231      242      +11     
- Partials      142      143       +1     

Impacted Files	Coverage Δ
PyPDF2/_writer.py	88.21% <76.92%> (-0.80%)	⬇️
PyPDF2/_reader.py	90.33% <81.48%> (-0.29%)	⬇️
PyPDF2/_utils.py	97.77% <88.23%> (-1.01%)	⬇️
PyPDF2/_merger.py	94.07% <94.52%> (-0.93%)	⬇️
PyPDF2/generic.py	91.57% <100.00%> (ø)
PyPDF2/types.py	100.00% <100.00%> (ø)
PyPDF2/_page.py	92.57% <0.00%> (+0.01%)	⬆️
PyPDF2/_encryption.py	85.98% <0.00%> (+0.58%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 844f238...b78fccd. Read the comment docs.

[MartinThoma]

Wow, very nice!

I've seen that you followed the the PyPDF2 deprecation process
🎉 👍

I think for types.BookmarkTypes and types.OutlinesType you also need to add aliases for a while. I would be ok if they don't have deprecation notices (I'm uncertain if those work for type annotations). You could just add a comment over the alias "# deprecated types:" or similar.

I've just noticed deprecate_bookmark and I love it 😍 (I still need to go through it in detail, though)

[mtd91429]

Good catch. I didn't even think about deprecating the types. I've amended the last commit with changes to address this issue.

I pushed a second commit to add unit tests for the decorator function. There are 3 (could be more, but I thought this was enough).

• one for the _writer module to show that the UserWarning is emitted.
• I added two to the _merger module:
  • one to show that the UserWarning is emitted
  • the other to show that the method invoked still produces the desired result (namely, to import the outline on the merged page still works with the deprecated keyword)

[mtd91429]

So many nits (all the forced pushes). I hope @MartinThoma you and the other repo managers don't get emailed for every failed CI like I do. If so, I'll be more thorough with unit testing on my end before pushing to GitHub. 😳

[MartinThoma]

Don't worry about that 😄

However, you don't need to make force-pushes. I will make a squashing commit in the end in any case and adjust the commit message. So it doesn't make a difference if you make normal pushes or force pushes.

[MartinThoma]

@mtd91429 I've adjusted your first comment so that it contains the deprecations. That comment will be used as a commit message when I squash the commits of this PR.

[MartinThoma]

@pubpub-zz @MasterOdin Do you want to have a look at the PR? To you agree with the renamings?

[pubpub-zz]

looks good
good job @mtd91429

[MasterOdin]

Fine with the rename, some slight review comments.

[MasterOdin]

Unrelated to the PR renaming, but this isn't a true replacement since there's no way to pass the optional parameters of this method to the property? Just no one uses them so it's fine?

[MasterOdin]

Personally, I'd rather see this call the replacement than the underlying internal function, just so if we decide to alter self.outline, then won't potentially need to also change self.outlines.

[MartinThoma]

Using the replacement also makes it more clear how to use the replacement / not use use the internal function. It's not a big overhead as well 👍

[MartinThoma]

@MasterOdin Thank you for the thorough review 🙏

[MartinThoma]

@mtd91429 I see that you've already spent a lot of time with this awesome PR / preparing it with the issue before. Thank you 🙏

All of the comments @MasterOdin added make a lot of sense to me. Especially the fact of moving some internal functions to the internal _utils.py module. Having many things only used internally / keeping the public interface slim allows us to change those quick without having to go through the deprecation process.

From my side, this PR is ready. I would appreciate it a lot if you incorporated the changes suggested by MasterOdin. However, if you prefer I can also do that after merging. Just let me know :-)

In any way, this will be merged soon :-)

[mtd91429]

@MasterOdin thank you for the thorough review as well with so many lines of code changed, I'm glad there were more eyes on this.

@MartinThoma I am perfectly happy to allow you to make any of the changes. I won't be able to do them until tomorrow, so don't wait for me. I also agree with @MasterOdin suggestions.

[MartinThoma]

I'm actually uncertain if I can do it before the weekend 😄 Let's see who is faster 😄 🚀

[MartinThoma]

@mtd91429 Thank you so much for all of the effort you put into this topic 🤗 You improved PyPDF2 🎉 ❤️

I'll make a release tomorrow :-)