2014-03-09 9:39 GMT+08:00 Darshit Shah <[email protected]>: > Hi Zihang, > > > I just had a brief glance through the whole commit. That's a very large > change! It's essentially the same code with lots of moving around and > cosmetic changes. > > However, I do have a couple of issues with it: > 1. I found it really difficult to follow the code. You should edit the > README file to reflect the current scenario and how should a developer > follow it. > 2. It seems like you've created some really nice abstractions, it would > very nice to explain them so the developers for Wget know what to look at > and what to edit. > 3. While the code surely is more pythonic, it creates a slight problem. > It's *more* pythonic. Most people who have to deal with this code are not > users who use Python everyday. I think, a little lesser of strict Python > syntax and a little more of simpler syntax will allow non-Python developers > to more easily follow the code. The point of using Python to rewrite the > old test suite was that Perl was a bit too cryptic and people had to spend > too much time understanding the code first before they could edit it. I > don't want to repeat that with having truly pythonic code which takes more > time to follow for a C developer. > I'm most sorry for the lack of documentation. I'll try adding some to README, CHANGELOG and the code itself (though I think some of the code is self-explaining itself, I'll add comments for non-pythoners, sorry again.)
> > Others, please chime in on this. I like the overall restructuring though. > And if the abstractions do work the way I think they do, I believe this > could be a good idea. I'll look at it in much more detail when I get the > time. > > > On Sun, Mar 9, 2014 at 2:22 AM, Darshit Shah <[email protected]> wrote: > >> Hi, >> >> Thanks for the refactoring. However, you've included makefile and >> makefile.in which are autogenerated files and should not be commited. >> >> Also, your patch has trailing whitespace errors. And I don't think you've >> added an entry to ChangeLog either. Please look into these. I haven't seen >> your patch yet. The Makefile errors mean I can't apply it without a lot of >> extra work. >> >> >> On Sat, Mar 8, 2014 at 5:38 PM, 陈子杭 (Zihang Chen) <[email protected]>wrote: >> >>> Hi. >>> >>> Here's a patch refactoring CommonMethods, HTTPTest and project hierarchy. >>> 19 tests passed, 1 xfailed as expected. >>> >>> Let me know if there's any problem. >>> >>> -- >>> Regards, >>> Chen Zihang, >>> Computer School of Wuhan University >>> --- >>> 此致 >>> 陈子杭 >>> 武汉大学计算机学院 >>> >> >> >> >> -- >> Thanking You, >> Darshit Shah >> >> > > > -- > Thanking You, > Darshit Shah > > -- Regards, Chen Zihang, Computer School of Wuhan University --- 此致 陈子杭 武汉大学计算机学院
