Slightly simplify imgformat retrieval #201

Closed
mara0004 wants to merge 2 commits from mara0004/img2pdf:imgformat into main
Contributor

No need for a loop here - we can access the enum like a dictionary.

No need for a loop here - we can access the enum like a dictionary.
mara0004 added 1 commit 2024-08-25 15:45:39 +00:00
No need for a loop here - we can access the enum like a dictionary,
which should be more efficient.
mara0004 changed title from nit: slightly simplify imgformat retrieval to Slightly simplify imgformat retrieval 2024-08-25 18:37:51 +00:00
Owner

No need for a loop here - we can access the enum like a dictionary.

Yes, if your python version is recent enough. Is there an objective reason to change this?

> No need for a loop here - we can access the enum like a dictionary. Yes, if your python version is recent enough. Is there an objective reason to change this?
Author
Contributor

Yes, if your python version is recent enough.

In the python docs, Enum __getitem__ has no versionadded note, so I don't think this is a recent addition? Moreover, I believe you're already using this syntax in other places of img2pdf, where dotted access would not work (e.g. Rotation["0"], Colorspace["CMYK;I"].

Is there an objective reason to change this?

Well, not that it matters much here, but the time complexity of a dict lookup should be O(1) whereas looping over the members is O(length). Also, the current loop seems to be lacking a break. So this is just a bit cleaner.

> Yes, if your python version is recent enough. In the python docs, Enum [`__getitem__`](https://docs.python.org/3/library/enum.html#enum.EnumType.__getitem__) has no versionadded note, so I don't think this is a recent addition? Moreover, I believe you're already using this syntax in other places of img2pdf, where dotted access would not work (e.g. `Rotation["0"]`, `Colorspace["CMYK;I"]`. > Is there an objective reason to change this? Well, not that it matters much here, but the time complexity of a dict lookup should be O(1) whereas looping over the members is O(length). Also, the current loop seems to be lacking a `break`. So this is just a bit cleaner.
Author
Contributor

Come to think of it, getattr(ImageFormat, imgdata.format, ImageFormat.other) would be even simpler than subscription in try/except.

Come to think of it, `getattr(ImageFormat, imgdata.format, ImageFormat.other)` would be even simpler than subscription in try/except.
mara0004 added 1 commit 2024-08-27 20:30:28 +00:00
This now a one-liner, compared to 6 lines originally.
And it should be (algorithmically) faster.
Author
Contributor

I updated the code to getattr(...) as drafted above. It's a one-liner now.

I updated the code to `getattr(...)` as drafted above. It's a one-liner now.
Owner

I updated the code to getattr(...) as drafted above. It's a one-liner now.

Thank you! Could you squash both commits into one?

> I updated the code to `getattr(...)` as drafted above. It's a one-liner now. Thank you! Could you squash both commits into one?
Owner

Hi, I squashed both commits into one for you and pushed the result into main.

Hi, I squashed both commits into one for you and pushed the result into main.
josch closed this pull request 2025-02-15 13:46:22 +00:00

Pull request closed

Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: josch/img2pdf#201
No description provided.