boazsegev / combine_pdf

A Pure ruby library to merge PDF files, number pages and maybe more...
MIT License
734 stars 155 forks source link

wrong description of rotate_left/right #158

Open MathieuDerelle opened 5 years ago

MathieuDerelle commented 5 years ago

the source code says

    # rotate the page 90 degrees counter clockwise
    def rotate_left

    # rotate the page 90 degrees clockwise
    def rotate_right

but from what I've tested, it's the opposite, rotate_left is rotating clockwise et rotate_right counter clockwise

SlavaEremenko commented 5 years ago

Confirming this issue! Version 1.0.16

boazsegev commented 5 years ago

@MathieuDerelle , thank you for opening this issue.

@SlavaEremenko , thank you for confirming this issue.

Although I haven't tested this issue yet, I suspect that the biggest question I will have with this issue is "how to fix it?":

  1. I can fix the documentation, making things a bit awkward, since rotate_left implies counterclockwise motion.

  2. I can fix the API implementation, breaking current implementations that rely on the rotation direction.

I think this change / fix will require a version bump due to it being a breaking change.

I'll think about this some more and would be happy to read your input.

Kindly, Bo.

MathieuDerelle commented 5 years ago

I think rotate_left and rotate_right do not make much sense.

If you imagine a circle, rotating left could mean

You could define those 2 methods : rotate_clockwise / rotate_counter_clockwise and deprecate rotate_left / rotate_right

SlavaEremenko commented 5 years ago

I agree with both of you. Keep the old methods, deprecate them, put a note into the docs and create new methods: rotate_cw and rotate_ccw.