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

[Zihang Chen]

Thank you very much for your advice!


2014-03-14 16:58 GMT+08:00 Yousong Zhou <address@hidden>:

> 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.
>
Yes, there are only new-style classes in Python3.

>  - 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.
>
Sure, I'll see what I can do.

>  - "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).
>
The first one is the one I think is named inappropriately (Retcode =>
RetCode).
But to maintain compatibility (ExpectedRetcode is still used in test case
scripts),
it's still in use utilizing register(alias='ExpectedRetcode').

>     2. The newly formatted error string will not run, since
> "expected_ret_code" is a int, and it will not format with "%s".
>
Actually, an int object will be converted to str automatically using
__str__ if
formatted with '%s'. (I use format int with '%s' all the time.)

>     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?
>
Yes, there is one. However using this feature will cause changes in the test
case scripts, because in the test cases {{port}} is used, while the format
syntax
is {port}. I'll think about it though.

>     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".
>
Yes! Didn't think about it. I think I'll rename register to "rule" and
"hook" for
different type of confs, though rule and hook are the same thing.

>     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.
>
Yes, I'll think about it.

>
>  - Double blank lines.
>
Ouch, I thought methods are separated with two blank lines in PEP8.
Looks like I'm wrong. I'll try fix this.

>  - 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.

Yes, I'll fix my editor for this. (I actually in my latest patches delete
the
newline at end of files on purpose. Looks like I got it wrong again.)

>
>
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.
>
> Thanks very much for these suggestions! I'll see what I can do.


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



-- 
Regards,
Chen Zihang,
Computer School of Wuhan University
---
此致
陈子杭
武汉大学计算机学院