Thank you very much for the review! I'll answer inline below. Den tis 3 dec. 2024 kl 22:31 skrev Jun Omae <jun6...@gmail.com>:
> Hi, > > On 2024/11/29 6:47, Daniel Sahlberg wrote: > [snip] > > > > The next step will be to rewrite for asyncio, but I'd like to have a > functional client as a baseline. > > > > Cheers, > > Daniel > > > > [1] > https://github.com/apache/infrastructure-pypubsub/blob/master/pypubsub.py > > I'm not familiar with svnpubsub.py but tried to run the script with the > proposed patch. > > However it doesn't work. Investigating it, 400 Bad Request is received > because > the client.py sends "GET HTTP/1.1" (missing request uri). After applying > the > following patch, it seems to work. > > [[[ > diff --git a/tools/server-side/svnpubsub/svnpubsub/client.py > b/tools/server-side/svnpubsub/svnpubsub/client.py > index 871a5e925..004003512 100644 > --- a/tools/server-side/svnpubsub/svnpubsub/client.py > +++ b/tools/server-side/svnpubsub/svnpubsub/client.py > @@ -75,7 +75,7 @@ class Client(asynchat.async_chat): > raise ValueError("URL scheme must be http: '%s'" % url) > host = parsed_url.hostname > port = parsed_url.port > - resource = parsed_url.path > + resource = parsed_url.path or '/' > if parsed_url.query: > resource += "?%s" % parsed_url.query > if parsed_url.fragment: > ]]] > > I think it would simplify by the following rather than `host + ":" + > str(port)`. > [[[ > - self.push(('GET %s HTTP/1.0\r\n\r\n' % resource).encode('ascii')) > + self.push(('GET %s HTTP/1.1\r\nHost: %s:%d\r\n\r\n' % > + (resource, host, port)).encode('ascii')) > ]]] > As Daniel Gruno noted, I was querying the wrong endpoint, so the HTTP/1.1 protocol isn't really needed. We might want to support both protocols at some point in time, but I'll leave these two enhancements for the future. > > In the minimal client, it uses `repr` but `__repr__` method of > `Notification` > class is not defined. I'd suggest to add the `__repr__` method. > [[[ > @@ -131,6 +140,8 @@ class Client(asynchat.async_chat): > class Notification(object): > def __init__(self, data): > self.__dict__.update(data) > + def __repr__(self): > + return '%s(%r)' % (self.KIND, self.__dict__) > > class Commit(Notification): > KIND = 'COMMIT' > ]]] > > Finally, `cc` and `mc` methods in the minimal client should have `url` > parameter. > [[[ > --- svnpubsub-client.py.orig 2024-12-04 06:25:33.083708157 +0900 > +++ svnpubsub-client.py 2024-12-02 10:28:30.489176287 +0900 > @@ -1,10 +1,10 @@ > -def cc(o): > +def cc(url, o): > print("commit " + repr(o)) > > def ec(url, type, o): > print("event on " + repr(url) + ": " + repr(type) + "\n" + repr(o)) > > -def mc(o): > +def mc(url, o): > print("metadata " + repr(o)) > > import client > ]]] > Good catch! With these two fixes, I now have a functioning client: [[[ event on 'http://svn.apache.org:2069/commits/': 'ping' 1734899014.037649 event on 'http://svn.apache.org:2069/commits/': 'ping' 1734899029.038485 commit http://svn.apache.org:2069/commits/: COMMIT({'committer': 'dsahlberg', 'log': 'Add a __repr__ function to allow repr() a Notification (used for a debugging client).\n\n* tools/server-side/svnpubsub/svnpubsub/client.py\n (Notifications): Add __repr__\n\nSuggested by: jun66j5', 'repository': '13f79535-47bb-0310-9956-ffa450edef68', 'format': 1, 'changed': {'subversion/trunk/tools/server-side/svnpubsub/svnpubsub/client.py': {'flags': 'U '}}, 'date': '2024-12-22 20:23:53 +0000 (Sun, 22 Dec 2024)', 'type': 'svn', 'id': 1922639}) event on 'http://svn.apache.org:2069/commits/': 'ping' 1734899044.039507 ]]] (The above is the client catching the commit of the __repr__ function). I'll continue with asyncio shortly. Cheers, Daniel
