Use bytes or unicode to get args #164
Conversation
Codecov Report
@@ Coverage Diff @@
## master #164 +/- ##
==========================================
+ Coverage 95.88% 96.03% +0.15%
==========================================
Files 12 13 +1
Lines 1530 1589 +59
Branches 87 88 +1
==========================================
+ Hits 1467 1526 +59
Misses 52 52
Partials 11 11
Continue to review full report at Codecov.
|
notoriousno
force-pushed
the
get-set-args
branch
from
4fd9c87
to
f3c3420
Compare
There was a problem hiding this comment.
Thanks for all your work on Klein! I do really like where you're going with this - an os.path-style compatibility approach to bytes vs. text here does make sense. However, I think it needs to be a bit better thought out (especially the compatibility contract around KleinSite before it's ready. Please address the feedback here and re-submit.
Thanks again!
|
|
||
| def getArg(self, key): | ||
| """ | ||
| Get a single arg value. |
There was a problem hiding this comment.
You forgot to document @param key, in particular @type key, which seems like rather the whole point of this change :).
|
|
||
| class KleinHTTPRequest(server.Request): | ||
|
|
||
| def getArg(self, key): |
There was a problem hiding this comment.
I really don't like the name getArg. "args" are an incredibly ill-defined mess in Twisted, mixing together query parameters ("GET args") and URL-encoded or multipart form-values in the body ("POST args" or "form args"). If we're going to try to do something better, it would be nice to clean this up a bit. Even if it is accessing the underlying .args, I feel like we should be disambiguating these namespaces.
There was a problem hiding this comment.
I'm not a big fan of getters and setters either but it was the simple and easy to understand. I had a more elaborate solution using a dict-like object for the args variable but other devs weren't convinced with it and ultimatly it was scraped in favor of getArgs. As far as the Twisted mess, you provided a ticket #228 but work on this might have stalled. Any hope of reviving that?
| Get a single arg value. | ||
|
|
||
| @raises KeyError: If key doesn't exist | ||
| @raises ValueError: If there is more than 1 value |
There was a problem hiding this comment.
You forgot about UnicodeDecodeError, a natural consequence of ensure_utf8_bytes.
|
|
||
|
|
||
|
|
||
| class KleinSite(server.Site): |
There was a problem hiding this comment.
Is this intended to be public? Do we need to use it for all Klein applications now? That seems like a pretty big change to quietly be foisting on all Klein applications. (That implies .app.resource() won't work in a normal server any more, or will randomly be missing some methods.)
If it is intended to be public, some docstrings seems like a bare minimum. Also, Site is an old-style class, so this should definitely have an , object at the end of its inheritance hierarchy.
There was a problem hiding this comment.
This is a rather ugly hack on my part and I'd like to move away from it. At the time I did not know of alternative ways to overload Request. In retrospect, I should've created an Request adapter or simply append the function to the request object in app.route(). Any thoughts on how to go about this?
| class KleinSite(server.Site): | ||
| def buildProtocol(self, addr): | ||
| channel = http.HTTPFactory.buildProtocol(self, addr) | ||
| channel.requestFactory = KleinHTTPRequest |
There was a problem hiding this comment.
Have you considered using the requestFactory argument to Site.__init__ rather than having a custom subclass?
|
Note that I've taken a stab at a new The goal would be to obsolete the @notoriousno: What are your thoughts on this? |
|
Normally I'm a fan of small, incremental improvements. Unfortunately, in this case, I think that we need a comprehensive re-design to make user education tractable. If we fix a sprawling core interface like So, given @wsanchez's work on #181, which I think has a better hope of becoming “The New Request Interface For Twisted”I think we would, sadly, be better off abandoning this work. Nevertheless, thank you, @notoriousno for submitting it. This is definitely a problem area and one that it's tough to work on. |
#157