Re: [Bug-wget] [GSoC] Refactoring the Test Suite
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. 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 dar...@gmail.com 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) chsc4...@gmail.comwrote: 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
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
Hi, Zihang and Darshit, and all. On 9 March 2014 09:39, Darshit Shah dar...@gmail.com wrote: 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. Hi, Zihang. The patch is really big as a single commit. You'd better split it into multiple small ones each for a single purpose, without breaking the code with each commit if possible. That way we can refer to and comment on the code more easily. 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. To be honest, I am fine with the current Perl implementation. My last several patches for the Perl-based test framework are my first try with Perl. It does not take me much time to understand the design and modify a few lines of code. I think documentation or self-explaining code is the solution. 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 dar...@gmail.com 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. Hi Zihang, looks like you development environment has to be configured right for this. The newline character in your code in now '\r\n' which should be '\n'. There are mode changes from 100755 to 100644 in the git commit, which is not right. Those .py files should retain their executable attributes. yousong
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
2014-03-09 9:39 GMT+08:00 Darshit Shah dar...@gmail.com: 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 dar...@gmail.com 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) chsc4...@gmail.comwrote: 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 --- 此致 陈子杭 武汉大学计算机学院
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
2014-03-09 11:31 GMT+08:00 Yousong Zhou yszhou4t...@gmail.com: Hi, Zihang and Darshit, and all. On 9 March 2014 09:39, Darshit Shah dar...@gmail.com wrote: 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. Hi, Zihang. The patch is really big as a single commit. You'd better split it into multiple small ones each for a single purpose, without breaking the code with each commit if possible. That way we can refer to and comment on the code more easily. 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. To be honest, I am fine with the current Perl implementation. My last several patches for the Perl-based test framework are my first try with Perl. It does not take me much time to understand the design and modify a few lines of code. I think documentation or self-explaining code is the solution. 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 dar...@gmail.com 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. Hi Zihang, looks like you development environment has to be configured right for this. The newline character in your code in now '\r\n' which should be '\n'. Yes, you're right. I mostly work under Windows. Didn't notice that. My bad. BTW is there anything similar to guideline for setting the development environment? Please let me know if there is one. Thanks, Yousong. There are mode changes from 100755 to 100644 in the git commit, which is not right. Those .py files should retain their executable attributes. yousong -- Regards, Chen Zihang, Computer School of Wuhan University --- 此致 陈子杭 武汉大学计算机学院
