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

Add --orientation option to pagerotate command #705

Merged
merged 6 commits into from
Apr 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ Release date: UNRELEASED

### New features and improvements

* ...
* Added a `--orientation` option to the `pagerotate` command to conditionally rotate the page to a target orientation (thanks to @gatesphere) (#705)

### Bug fixes

Expand Down
22 changes: 22 additions & 0 deletions tests/test_commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,28 @@ def test_pagerotate_error(caplog):
assert "page size is not defined, page not rotated" in caplog.text


def test_pagerotate_orientation():
doc = vpype_cli.execute("random pagesize a4 pagerotate -o landscape")
assert doc.page_size == pytest.approx((1122.5196850393702, 793.7007874015749))

doc = vpype_cli.execute("random pagesize a4 pagerotate -cw -o landscape")
assert doc.page_size == pytest.approx((1122.5196850393702, 793.7007874015749))

doc = vpype_cli.execute("random pagesize --landscape a4 pagerotate -o portrait")
assert doc.page_size == pytest.approx((793.7007874015749, 1122.5196850393702))

doc = vpype_cli.execute("random pagesize --landscape a4 pagerotate -cw -o portrait")
assert doc.page_size == pytest.approx((793.7007874015749, 1122.5196850393702))


def test_pagerotate_orientation_error():
doc = vpype_cli.execute("random pagesize a4 pagerotate -o portrait")
assert doc.page_size == pytest.approx((793.7007874015749, 1122.5196850393702))

doc = vpype_cli.execute("random pagesize --landscape a4 pagerotate -o landscape")
assert doc.page_size == pytest.approx((1122.5196850393702, 793.7007874015749))


def test_help(runner):
res = runner.invoke(cli, "--help")

Expand Down
34 changes: 31 additions & 3 deletions vpype_cli/operations.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,14 @@

from .cli import cli
from .decorators import global_processor, layer_processor
from .types import IntegerType, LayerType, LengthType, PageSizeType, multiple_to_layer_ids
from .types import (
ChoiceType,
IntegerType,
LayerType,
LengthType,
PageSizeType,
multiple_to_layer_ids,
)

__all__ = (
"crop",
Expand Down Expand Up @@ -662,12 +669,20 @@
is_flag=True,
help="Rotate clockwise instead of the default counter-clockwise",
)
@click.option(
"--orientation",
"-o",
type=ChoiceType(["portrait", "landscape"]),
help="Conditionally rotate only if the final orientation matches this option",
)
@global_processor
def pagerotate(document: vp.Document, clockwise: bool):
def pagerotate(document: vp.Document, clockwise: bool, orientation: str | None):
"""Rotate the page by 90 degrees.

This command rotates the page by 90 degrees counter-clockwise. If the `--clockwise` option
is passed, it rotates the page clockwise instead.
is passed, it rotates the page clockwise instead. If the `--orientation` option is given
a value of either 'portrait' or 'landscape', the page will only be rotated if the final
orientation would match that choice.

Note: if the page size is not defined, an error is printed and the page is not rotated.
"""
Expand All @@ -677,6 +692,19 @@
return document
w, h = page_size

# sanity checks
if orientation is not None and orientation not in ["portrait", "landscape"]:
logging.warning(

Check warning on line 697 in vpype_cli/operations.py

View check run for this annotation

Codecov / codecov/patch

vpype_cli/operations.py#L697

Added line #L697 was not covered by tests
f"pagerotate: orientation value '{orientation}' not one of 'portrait', 'landscape'"
)
return document

Check warning on line 700 in vpype_cli/operations.py

View check run for this annotation

Codecov / codecov/patch

vpype_cli/operations.py#L700

Added line #L700 was not covered by tests

# check orientation constraint, and do nothing if target orientation
# won't match desired result
if (orientation == "portrait" and h > w) or (orientation == "landscape" and w > h):
Copy link
Owner

Choose a reason for hiding this comment

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

A test for orientation is None is needed here because of the change above.

Copy link
Contributor Author

@gatesphere gatesphere Mar 7, 2024

Choose a reason for hiding this comment

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

Is an explicit test needed? If orientation == None, then orientation == "portrait" will evaluate False. The code works fine without an explicit test here.

Copy link
Owner

Choose a reason for hiding this comment

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

I'd be surprised if mypy agreed with that, but let's see :) In any case, I generally dislike such implicit tests for None. Also, arguably any other string input should be checked and trigger a logging.warning(), and that requires an explicit check for None.

Copy link
Owner

Choose a reason for hiding this comment

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

well apparently mypy is on your side :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mypy or not, I'm good either way. I can add it if you'd like.

Guido's 'explicit is better than implicit' perhaps applies here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Garbage input?

Copy link
Owner

Choose a reason for hiding this comment

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

I mean, if the use says pagerotate --orientation garbage. This should trigger a warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Click or ChoiceType is already doing that:

(vpype-py3.11) [redacted@redacted]$ vpype read dscope_liked_1.svg pagerotate -o garbage
Usage: vpype [OPTIONS] COMMAND1 [ARGS]... [COMMAND2 [ARGS]...]...

Error: Invalid value: 'garbage' is not one of 'portrait', 'landscape'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do realize now that we can't rely on click for people using it via the API...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a check to the most recent commit.

logging.debug("pagerotate: page already in target orientation, page not rotated")
return document

if clockwise:
document.rotate(math.pi / 2)
document.translate(h, 0)
Expand Down
Loading