-
Notifications
You must be signed in to change notification settings - Fork 308
Python3-six-removal #607
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
Python3-six-removal #607
Conversation
|
Ok there's something wrong with the CI itself |
patkan
left a comment
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.
Thank you for the contribution.
First review pass, I will check again with more time. Please have a look at the comments.
As far as I know the type annotation for a None return type is technically not necessary, but doesn't do any harm either, so why not.
It would be great if you would package for Debian, maybe we should discuss in a discussion what would be needed in order to do it from this repository (on a separate branch?)
this is against the common practices ... this way you would get a .deb file not a Debian package. Debian is still obsessed about .tar.gz and these would then be loaded on https://salsa.debian.org/ |
OK, we can do whatever is best practice. The only thing that would be important to me is that we find a way that could be "recoverable" if you would stop packaging python-escpos. I have tried (with the organization) to have the python-escpos package at least somehow hosted in a such a way that somebody else could step up and take this over with (hopefully) not too much trouble, should I stop doing this some day. (Ideally, there would be a transition, but you never know) |
|
I'm aware of this too strong ownership of packages in the Debian project. I got shouted at by some survivalist living of the grid 11 months in a row I will package it under the "Python team" with a disclaimer that anyone else can update it. |
That would be great :-) I have created a PR that refactors everything to f-strings: #608. If you do not mind merging or rebasing your branch, I would merge that into master. |
I do not use an ide ... still nano ... :-| |
I have checked and it is rather easy to merge. If that is OK for you, I would merge #608 and merge the changes into your branch, then you just have to pull :-) |
|
Yes please. |
Done |
|
Now there is only one thing left in the CI: I will soon be offline for today and cannot approve the CI runs. But you can also execute the tests locally with e.g. |
|
I think the issue was that one: 369e709, feel free to revert if that is not what you meant. |
|
I would have proposed a |
|
Your solution is the perfect one for python3.8, which I'm not used to work with (I do 2.6, 2.7, 3.7, 3.11, 3.12) |
OK, but then I would prefer that we git revert 369e709, because I would prefer more the way how it will be done in the future. |
This reverts commit 369e709.
I have cleaned up my commit |
pyupgrade can do it automaticaly for you when you bump your baseline.
most uppercase imports from but it's easier to keep it 3.8 coherent for now. |
this looks like a real bug |
Is this about the return value of the cups driver? If yes, maybe @belono can help? It looks like the return value of _read is not compatible with the rest of the lib for this driver. |
|
There's Escpos && EscposIO: One close() is missing |
|
It's a baseclass, so maybe just addig "def close(): pass" is enough |
|
I know nothing about win32, but the typing in the stubs feels wrong. |
|
BTW the |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #607 +/- ##
==========================================
- Coverage 80.98% 80.77% -0.22%
==========================================
Files 21 21
Lines 1620 1633 +13
Branches 288 286 -2
==========================================
+ Hits 1312 1319 +7
- Misses 226 233 +7
+ Partials 82 81 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Hi @a-detiste, are you ready with the change? (CI is passing)If yes, I would try to fit in a final review, but otherwise we can also wait of course. |
|
Hi, I would like to slow down before my Holydays (from 20), so let's wrap this up.
|
|
Ok, well this would be quite a lot still, haven't checked mypy directly. Should we merge this pr now? Then you can open a new one for the next steps when you get to it |
|
Yes please merge. I will track on my side the dependecies that needs:
|
|
I need to package python3-barcode first |
|
please include examples/ in mypy run. -tox.ini:commands = mypy src test |
patkan
left a comment
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.
For me these changes would be ok. @belono what do you think?
|
I left a comment in the cups.py and another one in test_printer_cups.py some days ago. 😄 Please, fix that before merge. |
Are you sure that you have sent the review? Could it be still pending? I cannot see those comments unfortunately. 😕 |
belono
left a comment
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.
Please, modify the CupsPrinter._read() method and its test.
I never got my device to work ... sadness,
I'm better at software that hardware.
I gifted identical device to a hackerlab and while googling for support I found this one back https://iooner.io/epson-vfd-hack/ :-) :-)
So anyway the weird mixture of Six & Typing ticked me and there I can help.
https://wiki.debian.org/Python3-six-removal
But ... ! While removing six I got scared and now I'd prefer to cleanup the annotations first.
(I can also package this for Debian)