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: Zihang Chen
Subject: Re: [Bug-wget] [GSoC] Refactoring the Test Suite
Date: Wed, 12 Mar 2014 22:14:33 +0800

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
---
此致
陈子杭
武汉大学计算机学院

Attachment: patches.tar.gz
Description: GNU Zip compressed data


reply via email to

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