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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

wolfram77
Copy link

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
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.

CHANGELOG.md Outdated
@@ -7,6 +7,7 @@
- New `uncompress` command ([PR #75](https://github.com/py-pdf/pdfly/pull/75))
- New `update-offsets` command to adjust offsets and lengths ([PR #15](https://github.com/py-pdf/pdfly/pull/15))
- New `rm` command ([PR #59](https://github.com/py-pdf/pdfly/pull/59))
- New `extract-annotated-pages` to filter out only the user annotated pages ([PR #98](https://github.com/py-pdf/pdfly/pull/98))
Copy link
Member

@Lucas-C Lucas-C Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please move this to a new section above?

## Version 0.5.0, not released yet

@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
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]

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