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.

Sign up for GitHub

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

Jump to bottom

ENH: Add --format Option for Custom Page Sizes in x2pdf Command #65

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

ENH: Add --format Option for Custom Page Sizes in x2pdf Command #65

wants to merge 11 commits into from

Conversation

mulla028
Copy link

Closes #63

Summary

This pull request introduces the --format option to the x2pdf command in pdfly, allowing users to specify custom page sizes for PDF output. The option supports standard formats such as Letter, A4-portrait, and A4-landscape, as well as custom dimensions (e.g., 210x297 mm). This enhancement provides flexibility for generating PDFs with various page sizes, scaling and centering images to fit each specified format.

Changes

Code Updates

  • cli.py

    • Added --format option to the x2pdf command with a default value of A4-portrait.
    • Updated help documentation to list available format options.

  • x2pdf.py

    • Implemented the get_page_size function to map standard format names to dimensions or parse custom dimensions (e.g., 210x297).
    • Updated image_to_pdf function to center and scale images according to the specified page size.
    • Modified main function to accept format as a parameter and apply the selected page size to each PDF page.

Tests

  • test_x2pdf.py
    • Added new test cases to validate the --format option with various formats, including standard (Letter, A4-portrait, A4-landscape) and custom dimensions (210x297).
    • Included a test to handle invalid format values, verifying appropriate error handling.

Verification Steps

  1. Run pdfly x2pdf --help to verify that --format is listed in the options.
  2. Test x2pdf with standard and custom formats: 
    pdfly x2pdf sample-files/sample1.jpg --output output_letter.pdf --format Letter
pdfly x2pdf sample-files/sample2.jpg --output output_a4.pdf --format A4-portrait
pdfly x2pdf sample-files/sample3.jpg --output output_custom.pdf --format 210x297

Lucas-C
Lucas-C reviewed Oct 30, 2024
View reviewed changes
pdfly/cli.py Outdated Show resolved Hide resolved
Lucas-C
Lucas-C reviewed Oct 30, 2024
View reviewed changes
pdfly/x2pdf.py Outdated Show resolved Hide resolved
Lucas-C
Lucas-C reviewed Oct 30, 2024
View reviewed changes
tests/test_x2pdf.py Outdated Show resolved Hide resolved
Lucas-C
Lucas-C reviewed Oct 30, 2024
View reviewed changes
tests/test_x2pdf.py
if format_option != "invalid-format":
assert exit_code == 0, captured
assert captured.out == ""
assert output.exists()
Copy link
Member

Choose a reason for hiding this comment

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

It would be interesting to also validate the resulting pages dimensions.

This can be checked with pdfly pagemeta $pdf_filepath $page_index

Or using the underlying pypdf library: PdfReader(pdf: Path).mediabox/.cropbox/.artbox/.bleedbox

Copy link
Member

Choose a reason for hiding this comment

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

I think you did not handle that feedback comment @mulla028 🙂

Lucas-C
Lucas-C reviewed Oct 30, 2024
View reviewed changes
pdfly/x2pdf.py Outdated Show resolved Hide resolved
Lucas-C
Lucas-C reviewed Oct 30, 2024
View reviewed changes
pdfly/x2pdf.py Outdated Show resolved Hide resolved
Lucas-C
Lucas-C requested changes Oct 30, 2024
View reviewed changes
Copy link
Member

@Lucas-C Lucas-C left a comment

Choose a reason for hiding this comment

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

Good job tackling this issue! 👍

Thank you for your contribution 🙂

I made a few comments as feedback.

@mulla028
Copy link
Author

Good job tackling this issue! 👍

Thank you for your contribution 🙂

I made a few comments as feedback.

Thank you for your feedback! I will do my best to make all these changes 🙂

@mulla028
Copy link
Author

@Lucas-C I tried to make all the requested changes. Review please, and let me know if you noticed any issues!

Lucas-C
Lucas-C reviewed Oct 31, 2024
View reviewed changes
pdfly/cli.py Outdated Show resolved Hide resolved
Lucas-C
Lucas-C reviewed Oct 31, 2024
View reviewed changes
pdfly/x2pdf.py Outdated Show resolved Hide resolved
@mulla028
Copy link
Author

mulla028 commented Oct 31, 2024 •
edited
Loading

@Lucas-C
Thank you for your feedback, some things were pretty new to me. Thank you for your patience. Hopefully, this time code follows your requirements 😄

@mulla028
Copy link
Author

mulla028 commented Nov 1, 2024

@Lucas-C is my problem comes with my code styling?

Lucas-C
Lucas-C reviewed Nov 1, 2024
View reviewed changes
pdfly/x2pdf.py
Comment on lines +27 to +28
elif orientation == "portrait":
return (width, height)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
elif orientation == "portrait":
return (width, height)

Just a minor code improvement suggestion.

This change is not mandatory before merging this PR.

@Lucas-C
Copy link
Member

Lucas-C commented Nov 1, 2024

@Lucas-C is my problem comes with my code styling?

No, it's OK 🙂

I only see a couple of blocking points:

FAILED tests/test_x2pdf.py::test_x2pdf_with_format - ValueError: Invalid or unsupported page format provided: Letter

You should be able to reproduce the problem by executing the unit tests on your own computer 🙂

@Lucas-C
Copy link
Member

Lucas-C commented Nov 20, 2024

Hi @mulla028 🙂

Just to get a quick update: do you still want to implement this feature?

@mulla028
Copy link
Author

Hi @mulla028 🙂

Just to get a quick update: do you still want to implement this feature?

Hi! Yes, I just had to do a lot of work for school. I am gonna finish implementing this issue by the end of this week.

@Lucas-C
Copy link
Member

Lucas-C commented Dec 2, 2024

Hi @mulla028

I'm sorry to ask again, but it has been 2 weeks, an another contributor has submitted a PR for a several feature: #77

Do you think that you will have some time in the near future to finish implementing this?

Else we could consider having someone else finishing the work on this PR, either me, @pastor-robert or another contributor 🙂

@Lucas-C Lucas-C mentioned this pull request Dec 2, 2024
Merged
@MartinThoma MartinThoma changed the title Add --format Option for Custom Page Sizes in x2pdf Command ENH: Add --format Option for Custom Page Sizes in x2pdf Command Dec 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lucas-C Lucas-C Lucas-C requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ENH: pdfly x2pdf --format
2 participants
@mulla028 @Lucas-C