WIP: Add some more flexibility #8
Conversation
|
Still working on this, you should expect a couple more updates. Comments welcome :) |
|
Oh dear, I wasn't thinking of configurability when I wrote this, only the ability to hack the code. I think that's come back to bite me now! |
There was a problem hiding this comment.
Most of the problems here are from each Behaviour being very general in theory, but all being minor variations in practice.
Perhaps it suggests that Behaviour is made of two parts: HowThePrinterWorks() and WhatYouDoWithThePDF().
There's just one HowThePrinterWorks. If you wanted to go a completely different route, this could be a .yaml.
And all the current behaviours are just WhatYouDoWithThePDF. Which would be nice to keep as argparse, because I find running ippserver run hexdump to be highly amusing!
| @@ -48,13 +48,21 @@ def prepare_environment(ipp_request): | |||
| return env | |||
|
|
|||
|
|
|||
| def env(var): | |||
| """Get (bytes) value from environment""" | |||
| value = os.environ.get(var) | |||
There was a problem hiding this comment.
I don't like the mix of argparse and os.environ, as it gives multiple ways to configure things at the same time.
But I can't think of another way to do this. (I think the "argparse kool aid" way would be to have a classmethod on the Behaviour which returns the subparser it would like? That feels over-complicated.)
So there's no good reason not do do it like this. Especially as the impact of forgetting to set IPP_PRINTER_NAME is very low.
|
|
||
| class PPD(object): | ||
| def text(self): | ||
| raise NotImplementedError() | ||
|
|
||
|
|
||
| class FilePPD(PPD): |
There was a problem hiding this comment.
We should probably make all the other PPD classes into normal files too - setup.py handles data_files, so this shouldn't be too bad. But I won't force you to do that as part of this PR!
| status=200, content_type='image/png' | ||
| ) | ||
| with open(local_file_location(self.path), 'rb') as wwwfile: | ||
| self.wfile.write(wwwfile.read()) |
There was a problem hiding this comment.
We should do something here to prevent self.path being some horror like "../../etc/passwd". Assert that path is a regexp pattern, or something?
|
The approach is generally fine. Hopefully this gives some extra ideas / encouragement if you wanted to do any bigger changes. The code is very much lacking in any design about how clients should use it |
Allows override of the printer name, description, icon, and PPD files - but keeping the great simplicity of the server.