Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Receipt logos: add support for black-and-white binary netpbm images. #282
Receipt logos: add support for black-and-white binary netpbm images. #282
Changes from 18 commits
b2bb96a
bce2bdb
ea9bb87
0dcc0cf
d72f0ee
c837eba
5da27b9
3808a1e
1f26bde
81de564
b09578a
4ce2d19
02b01a2
bf55022
a7e8902
9e335cc
87eca3b
6cca49c
1f41665
e5d8cbb
2ffafaf
e55f5fa
080c53c
10b0a17
71a8cb4
225edf6
46e97df
6e086e7
027b00b
e4634c5
d47cc3c
136c5b9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note-to-self: this division should be adjusted to use a math ceiling-function; there could (and in the case of 512h, is) be trailing line data otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more Pythonic way to do this would be to iterate over a generator that returns (up-to) 24 row chunks.
Eg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Much better 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the Python version support policy for the application? I see Py3.8+ mentioned in
setup.cfg
;itertools.batched
seems to be from Py3.12+ (still possible that a cleanup similar to this is possible using more backwards-compatible functions, though).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good point. Yes,
itertools.batched
is still too new to use. You could include a generator that does something suitable, and which also pads to a multiple of 24 rows?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(And to answer your question directly: I'm still officially supporting Python 3.8 [it's in Ubuntu 20.04 which I still use in a couple of places], but I expect I'll drop that for the next major release and require a minimum of Python 3.10.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha:
more-itertools
providesbatched
, and in fact may even be the origin of the Py3.12 implementation (although I'm not certain of that).It's Py3.8+ compatible, and using a similar implementation would make it easier to drop that dependency when the baseline here becomes Py3.12+ at a later date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the version of
more-itertools
in Ubuntu 22.04 (which the tills run on in my pub company) doesn't includebatched
. I think you're going to have to include the text of the function in this project. (It's very small.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh, that's frustrating. I noticed this comment after adding the dependency to the GitHub Workflows definition.. clearing that out in a moment.