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

ENH: With extract-annotated-pages command #98

Merged
merged 2 commits into from
Feb 17, 2025
Merged

Conversation

wolfram77
Copy link
Contributor

This pull request addresses the following issue:

ENH: Add command to extract annotated pages #97


@Lucas-C

  • even links (internal or external) are annotations!

This was the issue.

@wolfram77 wolfram77 changed the title With extract-annotated-pages command ENH: With extract-annotated-pages command Feb 6, 2025
@Lucas-C
Copy link
Member

Lucas-C commented Feb 7, 2025

Thank you for your contribution @wolfram77 👍

Could you please address the following points, and I'll be happy to merge your PR:

  • please run black pdfly/extract_annotated_pages.py so that the GitHub Actions CI pipeline passes
  • please add a mention of your addition to CHANGELOG.md as part of this PR
  • please include at lease one basic unit test in test/test_extract_annotations.py. You could take inspiration from the code snippet below to create a PDF file with annotations "on the fly" in this unit test:
from fpdf import FPDF

pdf = FPDF()
pdf.set_font("Helvetica", size=12)

pdf.add_page()
text = "Link set over an arbitrary area with FPDF.link()"
x, y = 20, 150
pdf.text(x=x, y=y, text=text)
width = pdf.get_string_width(text)
pdf.link(
    x=x,
    y=y - pdf.font_size,
    w=width,
    h=pdf.font_size,
    link="https://github.com/py-pdf/fpdf2/discussions",
)

pdf.add_page()
pdf.text_annotation(
    x=20,
    y=150,
    text=f"This is a default text annotation.",
)
pdf.output("pdfly_pr_98.pdf")

PS: I'll be on holiday for a few days, so I'll get back to you only mid-february.

@Lucas-C
Copy link
Member

Lucas-C commented Feb 7, 2025

@all-contributors please add @wolfram77 for code

Copy link
Contributor

@Lucas-C

I've put up a pull request to add @wolfram77! 🎉

@wolfram77
Copy link
Contributor Author

@Lucas-C Thanks for reviewing the PR. To simplify the test, I added a yellow highlight to page 7 of resources/input8.pdf. The test case now looks for one annotated page in it.

@Lucas-C
Copy link
Member

Lucas-C commented Feb 7, 2025

Nice, good job with the unit test 👍

I think there are issues with the unit tests, on the main branch, regardless of this PR.

I won't have the time to fix them today, so you can either have a look at it based on the GitHub Actions logs, or else I'll fix that when I'll be back from holiday.

@wolfram77
Copy link
Contributor Author

wolfram77 commented Feb 7, 2025

@Lucas-C From the first failed test, I see the following:

        # Assert
        captured = capsys.readouterr()
>       assert exit_code == 0, captured
E       AssertionError: CaptureResult(out='', err="\x1b[33mUsage: \x1b[0mpytest update-offsets [OPTIONS] FILE_IN
E         \x1b[2mTry \x1b[0m\x1b[2;34m'pytest update-offsets \x1b[0m\x1b[1;2;34m-\x1b[0m\x1b[1;2;34m-help\x1b[0m\x1b[2;34m'\x1b[0m\x1b[2m for help.\x1b[0m
E         \x1b[31m╭─\x1b[0m\x1b[31m Error \x1b[0m\x1b[31m─────────────────────────────────────────────────────────────────────\x1b[0m\x1b[31m─╮\x1b[0m
E         \x1b[31m│\x1b[0m Got unexpected extra argument                                                \x1b[31m│\x1b[0m
E         \x1b[31m│\x1b[0m (/tmp/pytest-of-runner/pytest-0/test_update_offsets0/file-with-offsets-out.p \x1b[31m│\x1b[0m
E         \x1b[31m│\x1b[0m df)                                                                          \x1b[31m│\x1b[0m
E         \x1b[31m╰──────────────────────────────────────────────────────────────────────────────╯\x1b[0m
E         ")
E       assert 2 == 0

I am not sure why it seems to expect update-offsets to have only FILE_IN as an argument (no FILE_OUT), so it is failing with Got unexpected extra argument error. Could it be a pytest / typer version issue? Would be best you take a look at it when you are back.

@Lucas-C
Copy link
Member

Lucas-C commented Feb 7, 2025

I fixed the main branch.

mypy reports some minor issues with your PR:

pdfly/extract_annotated_pages.py:20: error: "Path" has no attribute "with_stem"  [attr-defined]
pdfly/extract_annotated_pages.py:28: error: "PdfObject" has no attribute "__iter__" (not iterable)  [attr-defined]
pdfly/cli.py:348: error: Argument 2 to "main" has incompatible type "Optional[Path]"; expected "Path"  [arg-type]

@wolfram77
Copy link
Contributor Author

@Lucas-C please update me when you are back. I have (hopefully) addressed the issues you mentioned above.

@Lucas-C
Copy link
Member

Lucas-C commented Feb 17, 2025

@wolfram77 I updated your branch to rebase it and fix the last issue with ruff

@Lucas-C Lucas-C force-pushed the main branch 2 times, most recently from ca9000d to 505ab93 Compare February 17, 2025 15:40
Seems that the `| None` syntax is not valid with Python 3.8

Co-authored-by: Lucas Cimon <[email protected]>
@Lucas-C Lucas-C merged commit 407e6cd into py-pdf:main Feb 17, 2025
10 checks passed
@Lucas-C
Copy link
Member

Lucas-C commented Feb 17, 2025

Merged!

Thank you @wolfram77 👍 🙂

@wolfram77
Copy link
Contributor Author

Lucas-C added a commit that referenced this pull request Feb 19, 2025
Lucas-C added a commit that referenced this pull request Feb 19, 2025
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