Add --orientation option to pagerotate command #705
Conversation
…nally rotating a page to a target orientation.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #705 +/- ##
==========================================
- Coverage 94.71% 94.68% -0.04%
==========================================
Files 62 62
Lines 5756 5777 +21
Branches 1272 1275 +3
==========================================
+ Hits 5452 5470 +18
- Misses 187 189 +2
- Partials 117 118 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Very useful addition, thanks! Some minor comments, should be straightforward to fix (mores so than the failing doc build with which I'm currently struggling—unrelated to the present PR).
Edit: please also:
- add a test similar to
test_pagerotate()intest_commands.py - update CHANGELOG.md—do include "(thanks @gatesphere)"
| @@ -677,6 +686,12 @@ def pagerotate(document: vp.Document, clockwise: bool): | |||
| return document | |||
| w, h = page_size | |||
|
|
|||
| # 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): | |||
There was a problem hiding this comment.
A test for orientation is None is needed here because of the change above.
There was a problem hiding this comment.
Is an explicit test needed? If orientation == None, then orientation == "portrait" will evaluate False. The code works fine without an explicit test here.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
well apparently mypy is on your side :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I mean, if the use says pagerotate --orientation garbage. This should trigger a warning.
There was a problem hiding this comment.
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'.
There was a problem hiding this comment.
I do realize now that we can't rely on click for people using it via the API...
There was a problem hiding this comment.
Added a check to the most recent commit.
…of `pagerotate` command, per @abey79 - Added new tests for `pagerotate --orientation` - Updated CHANGELOG.md
|
Added '--orientation' option to pagerotate command, to allow conditionally rotating a page to a target orientation.
Description
Allows conditionally rotating a page with the pagerotate command by specifying a target orientation. If the rotated page would not match the target orientation, pagerotate does nothing. Defaults to 'any', default behavior is unchanged.
Checklist
blackandruff check .)mypyreturns no errorpytestsucceedshelpreference.rstmake clean && make htmlindocs/)