Thank you very much for your advice!
2014-03-14 16:58 GMT+08:00 Yousong Zhou <yszhou4t...@gmail.com>: > 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) <chsc4...@gmail.com> wrote: > > Hi. > > > > Just finished on my latest patches. > > > > > > 2014-03-12 22:49 GMT+08:00 Darshit Shah <dar...@gmail.com>: > > > >> 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) <chsc4...@gmail.com> > >> 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 <dar...@gmail.com>: > >> > > >> >> > >> >> > >> >> > >> >> On Mon, Mar 10, 2014 at 10:25 AM, 陈子杭 (Zihang Chen) < > chsc4...@gmail.com > >> > > >> >> 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) <chsc4...@gmail.com>: > >> >>> > >> >>>> > >> >>>> > >> >>>> > >> >>>> 2014-03-10 16:17 GMT+08:00 Darshit Shah <dar...@gmail.com>: > >> >>>> > >> >>>>> > >> >>>>> > >> >>>>> > >> >>>>> On Mon, Mar 10, 2014 at 8:46 AM, 陈子杭 (Zihang Chen) < > >> chsc4...@gmail.com> > >> >>>>> 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 <yszhou4t...@gmail.com>: > >> >>>>>> > >> >>>>>> > Hi, Zihang, > >> >>>>>> > > >> >>>>>> > On 10 March 2014 13:05, 陈子杭 (Zihang Chen) <chsc4...@gmail.com> > >> >>>>>> > 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 --- 此致 陈子杭 武汉大学计算机学院