bug-wget
[Top][All Lists]
Advanced

[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: Fri, 14 Mar 2014 16:58:11 +0800

Hi, Zihang.

You are making progress. :)  Below are my comments and hope that will
help you in some way.

 - I haven't got the chance to try python3, but in python2, there is
the difference between traditional classes and new-style classes, i.e.
"class C:" versus "class C(obj):". If it is still there in python3,
new-style classes is generally preferred.
 - In the newly introduced "conf" package, if it is your intention
that "gen_hook()" is not expected to be exported, then you may want to
enforce it in some way.
 - "Refactor a lot" is truly a lot for a single commit.  :)
 - I randomly check on the 8th patch.

    1. Why there are three names of almost the same meaning appearing
in the code, i.e. ExpectedRetcode, ExpectedRetCode, ExpectedReturnCode
(This one was found in the repository).
    2. The newly formatted error string will not run, since
"expected_ret_code" is a int, and it will not format with "%s".
    3. I vaguely remembered that a new format string syntax was
proposed and recommended.  While at it, is it possible that this
feature be used?
    4. If the decorator "register()" is about "conf", then "conf" or
"register_conf" should be a better name, for the same reason it is
"staticmethod" instead of "register_staticmethod".
    5. On the expected_files.py, files_crawled.py, etc. you may want
to format a proper error string and raise an exception with it instead
of "print" it to stdout.

 - Double blank lines.
 - Notice the "No newline at end of file" messages generated by git.
Vim will automatically add a newline at end of a file, not sure what's
your editor for this.

Below are my personal preferences on SubmittingPatches, pick some if
they seem to be reasonable.

 - Describe your overall plan and intention in a cover letter can be
good.  Try it with --cover-letter option when generating patches with
"git format-patch".
 - Cover letter can also be used to track differences between versions
of your patch series so that reviewers can easily know what's new in
this version. Try it with --subject-prefix='PATCH vN' option when
using "git format-patch".
 - The Subject line of your should be short and concise, fitted into
one line sentence without commas, and detailed description goes to
message body.
 - I prefer patches sent with "git send-email 000*.patch", that way we
can view and refer to the specific lines of the patch with context.

Link [1] may serve as a good example.  I can only find guide for
submitting patches to wget with Mercurial [2].

[1] http://www.linux-mips.org/archives/linux-mips/2011-01/threads.html#00006
[2] 
http://wget.addictivecode.org/PatchGuidelines#How_to_create_patches_with_Mercurial


                yousong

On 14 March 2014 10:16, 陈子杭 (Zihang Chen) <address@hidden> wrote:
> Hi.
>
> Just finished on my latest patches.
>
>
> 2014-03-12 22:49 GMT+08:00 Darshit Shah <address@hidden>:
>
>> Hi,
>>
>> That's great! Thanks for the patches. I do have a few comments about it
>> though:
>>
>> 1. You are still missing ChangeLog entries for each patch. You should
>> Ideally have a ChangeLog entry for every commit.
>> 2. I am not sure I completely follow the logic of the extra lines of
>> code that you've added to colour_terminal.py
>> 3. More importantly, you moved the module ColourTerm, but did not
>> change the import statements in any file.
>> Each commit should build successfully. That is why we have smaller
>> commits. A failure in this commit will give
>> false positives when someone attempts to use git bisect to find a
>> regression.
>> 4. Your commit message states, "move ColourTerm.py to
>> misc/colour_terminal.py" but you're doing more than that.
>>
>> I would suggest you move all the files you want in one commit and then
>> edit them later in different commits.
>> Please fix the patches so that every commit compiles and runs the
>> tests independently. A commit should not
>> depend on patches that come in the following commit.
>>
>>
>> On Wed, Mar 12, 2014 at 3:14 PM, 陈子杭 (Zihang Chen) <address@hidden>
>> wrote:
>> > Hi Darshit and Yousong,
>> >
>> > I think I finally got things straight.
>> >
>> > The big commit was split into 9 relatively small commits, and I removed
>> all
>> > the trailing backspaces and new lines. These patches apply to
>> > origin/parallel-wget without warnings.
>> >
>> > Thank both of you very much for all the help!
>> >
>> > If you have any concerns about the contents of the patches, please let me
>> > know.
>> >
>> >
>> > 2014-03-10 19:49 GMT+08:00 Darshit Shah <address@hidden>:
>> >
>> >>
>> >>
>> >>
>> >> On Mon, Mar 10, 2014 at 10:25 AM, 陈子杭 (Zihang Chen) <address@hidden
>> >
>> >> wrote:
>> >>>
>> >>> I applied dos2unix to all the files under testenv, checked with file
>> >>> command, deleted all pyc files, line wrap to 80 characters and format
>> a new
>> >>> patch. (I swear this will be the last huge patch I'll ever make.)
>> >>>
>> >>> I also git am this patch to a clean clone locally, and got two warning:
>> >>>     warning: squelched 16 whitespace errors
>> >>>     warning: 21 lines add whitespace errors.
>> >>> Is this ok?
>> >>>
>> >> I haven't checked the patch yet, but just a few suggestions:
>> >>
>> >> 1. You don't need to delete the pyc files locally. Simply don't add them
>> >> to the git commit. Use a local .gitignore file to handle it
>> >> 2. You can and should split this patch. I'm assuming it's the same stuff
>> >> as before, and that can be split. Use your imagination
>> >> 3. The whitespace errors imply trailing whitespace. This happens when yo
>> >> uhave extra whitespace characters at the end of a
>> >> line. Usually not a good idea sinec these are characters that cannot be
>> >> seen. You should eliminate them. My ViM editor
>> >> simply highlights all trailing whitespaces so I always know if they are
>> >> there. Also, you can configure your git to explicitly
>> >> highlight trailing whitespaces in its diff output (Assuming you're
>> using a
>> >> git shell, not a GUI, in which case I have no idea.)
>> >>
>> >>> Nervously, Chen
>> >>
>> >> Don't worry. Everyone faces problems with these items in the beginning.
>> >> It's not something you are used to.
>> >>
>> >>>
>> >>>
>> >>>
>> >>> 2014-03-10 16:34 GMT+08:00 陈子杭 (Zihang Chen) <address@hidden>:
>> >>>
>> >>>>
>> >>>>
>> >>>>
>> >>>> 2014-03-10 16:17 GMT+08:00 Darshit Shah <address@hidden>:
>> >>>>
>> >>>>>
>> >>>>>
>> >>>>>
>> >>>>> On Mon, Mar 10, 2014 at 8:46 AM, 陈子杭 (Zihang Chen) <
>> address@hidden>
>> >>>>> wrote:
>> >>>>>>
>> >>>>>> Hi Yousong,
>> >>>>>>
>> >>>>>> So sorry about the line endings, I'll have to do a thorough check.
>> >>>>>
>> >>>>> I'm not sure about the line endings since my git and vim
>> cinfiguration
>> >>>>> simply do the magic
>> >>>>> of conversions for me. But if Yousong says do, do look into it.
>> >>>>>
>> >>>>> However, you seem to have added a huge amount of those especially in
>> >>>>> your 2nd patch.
>> >>>>>
>> >>>>> I do however, very strongly suggest that you get access to some sort
>> of
>> >>>>> a linux system. It will
>> >>>>> make your life so much easier. Autoconf takes ages to run on Windows
>> in
>> >>>>> a cygwin shell.
>> >>>>>
>> >>>>>>
>> >>>>>>
>> >>>>>> BTW, the pyc files in 0001.patch was deleted in the second commit.
>> >>>>>
>> >>>>>
>> >>>>> It would be better if you just did not have them there. It woulld
>> >>>>> clutter *everyone's* git repos
>> >>>>> if the .pyc files were there and later deleted. Because git will
>> leave
>> >>>>> a snapshot of each
>> >>>>> commit in the history. Keep a .gitignore file handy. Those are very
>> >>>>> important. You'll get
>> >>>>> good ones for starts from github's own gitignore repository.
>> >>>>
>> >>>> Got it. But I wonder where to put the .gitignore file. Should I use
>> the
>> >>>> one in the `wget` directory or
>> >>>> get a new one under `testenv`?
>> >>>>>
>> >>>>>
>> >>>>>
>> >>>>> Also, we usually expect a ChangeLog entry for *every* patch being
>> sent.
>> >>>>> So, please keep that
>> >>>>> in mind too. And there's also the 80 characters per line limit we
>> like
>> >>>>> to follow for all files.
>> >>>>>
>> >>>> I'll keep that in mind.
>> >>>>>
>> >>>>> The chief reason was that older terminals could only display 80
>> >>>>> characters. Now, the reason is
>> >>>>> that it allows you to have two vertical windows with code
>> >>>>> simultaneously without any line wraps.
>> >>>>>
>> >>>>> And do follow Yousong's advice on organizing your patchset. Ask for
>> >>>>> help and you shall get it.
>> >>>>> Large, single commits are seldom looked upon favourably.
>> >>>>>>
>> >>>>>>
>> >>>> I'll try to make my commits smaller next time. Work till now I is not
>> >>>> likely to be divided into small
>> >>>> commits though ;(
>> >>>>>>
>> >>>>>> And thanks very much for the advice!
>> >>>>>>
>> >>>>>>
>> >>>>>> 2014-03-10 15:38 GMT+08:00 Yousong Zhou <address@hidden>:
>> >>>>>>
>> >>>>>> > Hi, Zihang,
>> >>>>>> >
>> >>>>>> > On 10 March 2014 13:05, 陈子杭 (Zihang Chen) <address@hidden>
>> >>>>>> > wrote:
>> >>>>>> > > Hi, Darshit.
>> >>>>>> > > I fixed the line ending using git config --global autocrlf
>> input.
>> >>>>>> > > Line
>> >>>>>> > > endings should be lf now. I also added some documentation. File
>> >>>>>> > > modes for
>> >>>>>> > > Test-*.py are 755 now.
>> >>>>>> > >
>> >>>>>> >
>> >>>>>> > I just did a quick check on the patch and the line endings are
>> still
>> >>>>>> > wrong, e.g. testenv/test/http_test.py
>> >>>>>> >
>> >>>>>> > Also, .pyc files should not be included, right?
>> >>>>>> >
>> >>>>>> > I do not have much experience with parallel-wget, but you can
>> >>>>>> > enhance
>> >>>>>> > organizing your commits by following how existing ones in the
>> >>>>>> > repository were written.
>> >>>>>> >
>> >>>>>> >
>> >>>>>> >                yousong
>> >>>>>> >
>> >>>>>>
>> >>>>>>
>> >>>>>>
>> >>>>>> --
>> >>>>>> Regards,
>> >>>>>> Chen Zihang,
>> >>>>>> Computer School of Wuhan University
>> >>>>>> ---
>> >>>>>> 此致
>> >>>>>> 陈子杭
>> >>>>>> 武汉大学计算机学院
>> >>>>>
>> >>>>>
>> >>>>>
>> >>>>>
>> >>>>> --
>> >>>>> Thanking You,
>> >>>>> Darshit Shah
>> >>>>>
>> >>>>
>> >>>>
>> >>>>
>> >>>> --
>> >>>> Regards,
>> >>>> Chen Zihang,
>> >>>> Computer School of Wuhan University
>> >>>> ---
>> >>>> 此致
>> >>>> 陈子杭
>> >>>> 武汉大学计算机学院
>> >>>>
>> >>>
>> >>>
>> >>>
>> >>> --
>> >>> Regards,
>> >>> Chen Zihang,
>> >>> Computer School of Wuhan University
>> >>> ---
>> >>> 此致
>> >>> 陈子杭
>> >>> 武汉大学计算机学院
>> >>>
>> >>
>> >>
>> >>
>> >> --
>> >> Thanking You,
>> >> Darshit Shah
>> >>
>> >
>> >
>> >
>> > --
>> > Regards,
>> > Chen Zihang,
>> > Computer School of Wuhan University
>> > ---
>> > 此致
>> > 陈子杭
>> > 武汉大学计算机学院
>> >
>>
>>
>>
>> --
>> Thanking You,
>> Darshit Shah
>>
>
>
>
> --
> Regards,
> Chen Zihang,
> Computer School of Wuhan University
> ---
> 此致
> 陈子杭
> 武汉大学计算机学院



reply via email to

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