orthosie
orthosie copied to clipboard
maxolasersquad
Printer: added support for epson printers using ESCPOS
@maxolasersquad Can you please comment on this when you have a moment with regards to the file structure and overall code. I'm new to django dev so I'm unsure what the convention is for files included into models.py (or if what I've done is a good idea at all!)
The PR adds support for the python-escpos EPSON printer divers (https://github.com/manpaz/python-escpos) while leaving the body of the existing code mostly alone and maintaining backwards compatibility with the current settings file format.
Note: I do not have a printer which is compatible with the original code to test my changes (only a networked EPSON printer)
Thanks, Sam
Yes, this looks great and I very much appreciate your help on this project. The code looks good to me. I would like to ask three things of you for this PR.
- Do all the unit tests still pass? I don't think I have much testing coverage of this part of the code so I don't think it will affect it.
- Did you use any tool to validate that is passes PEP-8 standards? I'm trying to make sure all future code is PEP-8 compliant.
- It's kind of tricky with hardware support, but if you see any opportunities to add some testing around this code it would be great, but don't stress about it.
Cool, no worries. I submitted this pull request before really finishing up the code to gauge interest, so I'll go ahead and clean it up a bit. To answer your questions:
- I did not at this stage, but I will have a look at the test code and give it a run. I also see you have added Travis support.
- I did not but now have a tool: autopep8
- Will do!
With #22 merged this branch will pass the auto test locally (although running python 2.7)
@maxolasersquad What are you thoughts on making a libs folder (or similar) in the root directory so that all apps have a common import location for stuff like this? The printer_interface.py file could reside there and could be imported if a __init__.py was in the folder with it.
I'm still working on the Travis CI integration, which is why it's always reporting failures at this point. I was trying to get it working before I started work this morning, but was not successful. Perhaps I'll have it done tonight.
No worries. I'll let you know when I've cleaned up some of the code a little.
As a note: I was having issues with the latest pip version of python-escpos when trying to print images. I'm currently using the master git branch without issue, so I'm not sure how that would impact Travis.
I got all of the travis-ci integration completed. Rebase your branch off of master to get all of the latest commits.
I’m a bit of a git novice so I'm sure that was a little harder than it should have been! I think I'll have to make some pre-commit scripts to identify errors. At least its passing now :)
@maxolasersquad I'm happy with how this is looking now. Let me know what you think.
Note: I have removed the examples from the end of the file because I feel that the docs are a better place for them. I have started a "printer" page in the wiki.
I have some thoughts and suggestions, but I want to hold off on them until I have fully reviewed the code and also reviewed how some of the existing logic works. I have an exceptionally busy week and probably won't get around to this until next week.
I want to apologize for how long it took me to review this. I got caught up contributing to some other projects, and my normal Friday-evening time I set aside for this got pushed out to do other things and fix other problems at the food coop.
I really like the way that you engineered the printer classes. I think it works very well.
There are a few things I'd like to change.
- This definitely needs to be in some module or app. For right now the printer is only used for a receipt printer, but there is no reason why at some point it couldn't be used for other purposes, but I'm not too sure what they would be. If you'd rather create a "printer" app to keep these libraries I wouldn't have a problem with that.
- Remove the constructor pieces that support the existing calls to Printer and change those calls to use the new layout.
- Have the register print the receipts based on the configuration in settings.py. Have the default settings be like the settings on line 109.
I'll put these specific comments directly in the merge request.
Oh, and rebase your branch against master to get rid of the merge conflict.
I was hoping you'd be interested in addressing these last comments so we could get this merged in.
Sorry... I had dropped this for a little while. Will address the above and re-submit the PR.