[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Bug-wget] [GSoC] Refactoring the Test Suite

From: Yousong Zhou
Subject: Re: [Bug-wget] [GSoC] Refactoring the Test Suite
Date: Sun, 9 Mar 2014 11:31:06 +0800

Hi, Zihang and Darshit, and all.

On 9 March 2014 09:39, Darshit Shah <address@hidden> 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 <address@hidden> 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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]