Update to Python3/modernise project #7
Conversation
…abases, under Postgres, etc
There was a problem hiding this comment.
Hey Josh! I've had a look over this and it all looks very sensible. Really great to get it onto a modern stack :)
How much testing have you done on this? I'm tempted to push it to our Heroku and see what happens... have you tested printers connecting to it in your docker set up? I'm thinking there might be some nasty str->bytes Python 3 stuff in the protocol code...
sirius/coding/test_image_coding.py
Outdated
| @@ -31,7 +31,7 @@ def test_full(self): | |||
|
|
|||
| image = Image.open(data) | |||
| image = image_encoding.crop_384(image) | |||
| image = image_encoding.threshold(image) | |||
| # image = image_encoding.threshold(image) | |||
There was a problem hiding this comment.
Any particular reason for removing this test?
There was a problem hiding this comment.
Uhhh, it was probably causing something to break and I forgot about it 😅 I'll give it a look!
There was a problem hiding this comment.
Reverted this in bcd45c3
I hadn't at all, was really more at a WIP level of "let's see what happens". I just pushed a couple of commits that seem to fix the core loop, so it...works? I think? Ish? (?!) I'm not sure if I mentioned at all in a previous issue/PR, but I've been building out a new client lib, so by default it'll just print images to the console (with pluggable adapters for different printers), and here's what it's doing after I pushed those commits:
I'm on the confident side of "maybe it works", but I really haven't poked at the corners of the API endpoints very much either, so likely some lurking issues in there. |
|
Well, I just tried pushing this PR to heroku and nothing seemed to break, so good job! I tested Twitter login, printing test messages and using print keys, all seemed to work great. Are you happy for this PR to be merged then? Anything else you'd like to do before calling it 'done'? |
|
I'm still finding some small issues here and there (as predicted, bytes vs strings) so perhaps hold off for a little longer and I'll nudge a few more corners. Hold that thought! |
|
Ok, I haven't found anything else, so let's kick the tyres some more on production? I'm gonna replace some deprecated things next (i.e. phantomjs), but can write tests specifically around those bits going forward. |
|
Merged! Working well so far. I had to adjust the callback URLs on the twitter app, but that seems to be working. Good job! |
Doh, I thought I kept them the same, but good to know it was easy to fix! |
Hiya,
This is a bit of a jumble, so let's call it a WIP for now. Broadly I'm not much of a Python developer, so I might need a little handholding.
I don't think it touches any of the models/DB handling, so it should overall be pretty safe.
My goal here isn't to mess with The Order, but I want a nice, stable space where I can add features (I want to add offline queuing!)
There are a couple of sidenotes to this:
postgresql.ARRAY(sa.String())in3a2c72f262b5_add_print_key_usage.py. I didn't want to touch that migration, since it'd break backward compatibility with live server(s). (small TODO: strip out all mentions of SQLite!)prettierdecided needing cleaning when I bumped them, so there's a bit of noise in the PR. I can revert that if you'd like a cleaner PR (but also: maybe we could add a code formatter for the Python at some point too? Black? That seems like a contentious decision!)runtime.txtbut I'm not actually sure if that's correct or not.