Skip to content

Conversation

@a-detiste
Copy link
Contributor

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)

@a-detiste
Copy link
Contributor Author

Ok there's something wrong with the CI itself

Copy link
Member

@patkan patkan left a 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?)

@a-detiste
Copy link
Contributor Author

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/
(to create another .tar.gz for the build servers :-) )

@patkan
Copy link
Member

patkan commented Dec 10, 2023

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/ (to create another .tar.gz for the build servers :-) )

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)

@a-detiste
Copy link
Contributor Author

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
and going online once a year. "It's my package"

I will package it under the "Python team" with a disclaimer that anyone else can update it.

@patkan
Copy link
Member

patkan commented Dec 10, 2023

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 and going online once a year. "It's my package"

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.

@a-detiste
Copy link
Contributor Author

If you do not mind merging

I do not use an ide ... still nano ... :-|

@patkan
Copy link
Member

patkan commented Dec 10, 2023

If you do not mind merging

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 :-)

@a-detiste
Copy link
Contributor Author

Yes please.

@patkan
Copy link
Member

patkan commented Dec 10, 2023

Yes please.

Done

@patkan
Copy link
Member

patkan commented Dec 10, 2023

Now there is only one thing left in the CI:

  py38: commands[0]> pytest
  ImportError while loading conftest '/home/runner/work/python-escpos/python-escpos/test/conftest.py'.
  test/conftest.py:4: in <module>
      from escpos.printer import LP, CupsPrinter, Dummy, File, Network, Serial, Usb, Win32Raw
  .tox/py38/lib/python3.8/site-packages/escpos/printer/__init__.py:4: in <module>
      from .cups import CupsPrinter
  .tox/py38/lib/python3.8/site-packages/escpos/printer/cups.py:16: in <module>
      from ..escpos import Escpos
  .tox/py38/lib/python3.8/site-packages/escpos/escpos.py:1363: in <module>
      class EscposIO:
  .tox/py38/lib/python3.8/site-packages/escpos/escpos.py:1441: in EscposIO
      self, type: type[BaseException], value: BaseException, traceback: TracebackType
  E   TypeError: 'type' object is not subscriptable

I will soon be offline for today and cannot approve the CI runs. But you can also execute the tests locally with e.g. tox -e py310 if you want to try out changes.

@patkan
Copy link
Member

patkan commented Dec 10, 2023

I think the issue was that one: 369e709, feel free to revert if that is not what you meant.

@a-detiste
Copy link
Contributor Author

I would have proposed a from __future__ import annotations

@a-detiste
Copy link
Contributor Author

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)

@patkan
Copy link
Member

patkan commented Dec 10, 2023

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.

@patkan
Copy link
Member

patkan commented Dec 10, 2023

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.

I have cleaned up my commit

@a-detiste
Copy link
Contributor Author

a-detiste commented Dec 10, 2023

I would prefer more the way how it will be done in the future.

pyupgrade can do it automaticaly for you when you bump your baseline.

Optional[a] becomes a | None
Union[a|b] becomes a | b

most uppercase imports from typing are replaced by lowercase ones from standard library
List -> list

but it's easier to keep it 3.8 coherent for now.

@a-detiste
Copy link
Contributor Author

src/escpos/escpos.py:1278: error: Incompatible return value type (got "bytes", expected "list[int]") [return-value]

this looks like a real bug

@patkan
Copy link
Member

patkan commented Dec 12, 2023

src/escpos/escpos.py:1278: error: Incompatible return value type (got "bytes", expected "list[int]") [return-value]

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.

@a-detiste
Copy link
Contributor Author

There's Escpos && EscposIO:

One close() is missing

class Escpos

    def __del__(self):
        """Call self.close upon deletion."""
        self.close()


class EscposIO:
    r"""ESC/POS Printer IO object.
       def __init__(
        self, printer: Escpos, autocut: bool = True, autoclose: bool = True, **kwargs
    ) -> None:
        """Initialize object.
        :param printer: An EscPos-printer object
        """
        self.printer = printer

 def close(self) -> None:
        """Close printer.

        Called upon closing the `with`-statement.
        """
        self.printer.close()

@a-detiste
Copy link
Contributor Author

It's a baseclass, so maybe just addig "def close(): pass" is enough

@a-detiste
Copy link
Contributor Author

I know nothing about win32, but the typing in the stubs feels wrong.

/usr/lib/python3/dist-packages/win32-stubs/win32print.pyi:def WritePrinter(hprinter: _win32typing.PyPrinterHANDLE, buf: str): ...

@a-detiste
Copy link
Contributor Author

BTW the ) -> None: has one specific purpose: it tells mypy that annotation job has been done and it can proceed with further checking. This makes more hints show up.

@codecov
Copy link

codecov bot commented Dec 13, 2023

Codecov Report

Merging #607 (b634354) into master (06bdb56) will decrease coverage by 0.22%.
The diff coverage is 90.32%.

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     
Flag Coverage Δ
unittests 80.58% <90.32%> (-0.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/escpos/__init__.py 100.00% <100.00%> (ø)
src/escpos/capabilities.py 83.69% <100.00%> (+0.17%) ⬆️
src/escpos/cli.py 78.26% <100.00%> (-0.24%) ⬇️
src/escpos/config.py 82.97% <100.00%> (ø)
src/escpos/constants.py 98.83% <100.00%> (ø)
src/escpos/exceptions.py 66.37% <100.00%> (+0.29%) ⬆️
src/escpos/image.py 100.00% <100.00%> (ø)
src/escpos/katakana.py 80.00% <100.00%> (ø)
src/escpos/printer/dummy.py 100.00% <100.00%> (ø)
src/escpos/printer/file.py 97.72% <100.00%> (+0.05%) ⬆️
... and 8 more

@patkan
Copy link
Member

patkan commented Dec 14, 2023

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.
Would it be possible to change the mypy ci step to be more strict? (Or do we have remaining issues there?)

@a-detiste
Copy link
Contributor Author

a-detiste commented Dec 14, 2023

Hi, I would like to slow down before my Holydays (from 20), so let's wrap this up.

mypy --strict generates 400 errors, it's way too early.

@patkan
Copy link
Member

patkan commented Dec 15, 2023

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

@a-detiste
Copy link
Contributor Author

Yes please merge.

I will track on my side the dependecies that needs:

  • annotations
  • packaging in Debian

@belono belono requested review from belono and patkan December 15, 2023 11:16
@a-detiste
Copy link
Contributor Author

I need to package python3-barcode first

@a-detiste
Copy link
Contributor Author

please include examples/ in mypy run.
this helped me understanding & fixing this previous typing error

-tox.ini:commands = mypy src test
+tox.ini:commands = mypy src test examples

-    def __init__(
-        idVendor: str = "",
-        idProduct: str = "",
-        usb_args: Dict[str, str] = {},
    
+    def __init__(
+        self,
+        idVendor: Optional[int] = None,
+     idProduct: Optional[int] = None,
+     usb_args: Dict[str, Union[str, int]] = {},

Copy link
Member

@patkan patkan left a 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?

@belono
Copy link
Contributor

belono commented Dec 15, 2023

I left a comment in the cups.py and another one in test_printer_cups.py some days ago. 😄

Please, fix that before merge.

@patkan
Copy link
Member

patkan commented Dec 15, 2023

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. 😕

Copy link
Contributor

@belono belono left a 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.

@patkan patkan merged commit 66a2e78 into python-escpos:master Dec 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants