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

[Zihang Chen]

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

patches.tar.gz
Description: GNU Zip compressed data