Hi. Just finished on my latest patches.
2014-03-12 22:49 GMT+08:00 Darshit Shah <[email protected]>: > 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) <[email protected]> > 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 <[email protected]>: > > > >> > >> > >> > >> On Mon, Mar 10, 2014 at 10:25 AM, 陈子杭 (Zihang Chen) <[email protected] > > > >> 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) <[email protected]>: > >>> > >>>> > >>>> > >>>> > >>>> 2014-03-10 16:17 GMT+08:00 Darshit Shah <[email protected]>: > >>>> > >>>>> > >>>>> > >>>>> > >>>>> On Mon, Mar 10, 2014 at 8:46 AM, 陈子杭 (Zihang Chen) < > [email protected]> > >>>>> 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 <[email protected]>: > >>>>>> > >>>>>> > Hi, Zihang, > >>>>>> > > >>>>>> > On 10 March 2014 13:05, 陈子杭 (Zihang Chen) <[email protected]> > >>>>>> > 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
