[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
|
From: |
Darshit Shah |
|
Subject: |
Re: [Bug-wget] [GSoC] Refactoring the Test Suite |
|
Date: |
Tue, 8 Apr 2014 01:24:57 +0200 |
Hi,
I'm going to be nitpicking, so bear with me:
1. See code [1]. Why is the import statement for HTTPServer being moved? I
don't see any functional use of this.
2. The colour_teminal.py file you create has no newline at the end. This
was pointed out earlier too if I remember correctly.
3. I'm no longer listing multiple "No newline at end of file" issues. I'm
going to assume you'll hunt them all down and fix them.
4. This looks all valid and pythonic, but why are we doing all this again?
Is there some logical reason why splitting everything into so many files is
important? I'd say when things are small and manageable, why not let it all
rest in a single file? I'm no expert with Python, but this just seems like
overkill to me. I'm looking at patch 5, and you've got loads of new files
which are all 10 lines each, including import statements and comments. Why
can't we use a single file for them all?
In fact, I'd say, that with a scripting language this is highly inefficient
since you're now reading lot sof files from disk again and again and again.
5. I haven't thoroughly looked through patch 5 yet. I'd honestly like to
discuss and understand why so many files need to be created before moving
on from here.
6. Again, I don't see any reason why you'd create a new file with only 2
lines on code and will be used only once. [2]
7. See [3]. Why does it have to be '../'. Isn't that non-portable syntax?
Since Windows happens to use \ for directories. Or did I miss something?
8. A lot of your patches seem redundant editing the same file twice in the
same location. Why not just do it once? You first split things into
constants.py and imported them. Then, two patches later you change the
import statements.
9. I'd suggest you update the README file along with each patch that makes
changes so that we never have a stale README version.
10. Patch 10 is purely aesthetic. In fact, so aesthetic, that it has no
changes to the naked, untrained eye. A full patch that only adds
whitespaces? Why not just fix them in the earliest patch causing the issue?
P.S.: If you're going to send multiple versions of such a long patchset, it
would be a really good idea to also tell us what has changed between the
last version and this. That will allow us to look at the right places
better.
[1]:
> import shlex
> import sys
> import traceback
> -import HTTPServer
> import re
> import time
> from subprocess import call
> -from ColourTerm import printer
> from difflib import unified_diff
>
> +import HTTPServer
> +from misc.colour_terminal import print_red, print_green, print_blue
> +
>
[2]:
> diff --git a/testenv/misc/constants.py b/testenv/misc/constants.py
> new file mode 100644
> index 0000000..5fad2f8
> --- /dev/null
> +++ b/testenv/misc/constants.py
> @@ -0,0 +1,3 @@
> +
> +HTTP = "HTTP"
> +HTTPS = "HTTPS"
>
>
[3]:
> - CERTFILE = os.path.abspath (os.path.join ('..', 'certs',
> 'wget-cert.pem'))
> + CERTFILE = os.path.abspath (os.path.join ('../', 'certs',
> 'wget-cert.pem'))
>
On Tue, Apr 1, 2014 at 9:41 AM, 陈子杭 (Zihang Chen) <address@hidden>wrote:
> Thanks very much in advance for paying attention to this newbie patch
> series!
>
>
> 2014-04-01 15:39 GMT+08:00 陈子杭 (Zihang Chen) <address@hidden>:
>
> > Hi everyone.
> >
> > Sorry for being late on amending my previous patches.
> >
> > I here upload version 2 of the patches.
> >
> > Changes since v1:
> > * Uppercase git commit lines in order to be consist with the older
> commits.
> > * Reduce lengths of commit lines and items in ChangeLog.
> >
> > P.S. I chose not to use git-send-email, it's too slow.
> >
> > Zihang Chen (11):
> > Create package misc, move ColourTerm.py to misc
> > From WgetTest.py move WgetFile to misc
> > Fix a typo in Test-Proto.py
> > Create package exc and move TestFailed to exc
> > Create package conf where rules and hooks are put
> > Move server classes to package server.protocol
> > Create package test for test case classes
> > Refactor mainly the test cases classes
> > Update README
> > Ensure line feed and blank line between methods
> > In conf, rename register to rule and hook
> >
> > testenv/ChangeLog | 144 ++++++++
> > testenv/ColourTerm.py | 23 --
> > testenv/FTPServer.py | 162 ---------
> > testenv/HTTPServer.py | 467
> > --------------------------
> > testenv/README | 81 +++--
> > testenv/Test--https.py | 6 +-
> > testenv/Test--spider-r.py | 3 +-
> > testenv/Test-Content-disposition-2.py | 3 +-
> > testenv/Test-Content-disposition.py | 3 +-
> > testenv/Test-Head.py | 3 +-
> > testenv/Test-O.py | 3 +-
> > testenv/Test-Parallel-Proto.py | 6 +-
> > testenv/Test-Post.py | 3 +-
> > testenv/Test-Proto.py | 6 +-
> > testenv/Test-auth-basic-fail.py | 3 +-
> > testenv/Test-auth-basic.py | 3 +-
> > testenv/Test-auth-both.py | 3 +-
> > testenv/Test-auth-digest.py | 3 +-
> > testenv/Test-auth-no-challenge-url.py | 3 +-
> > testenv/Test-auth-no-challenge.py | 3 +-
> > testenv/Test-auth-retcode.py | 3 +-
> > testenv/Test-auth-with-content-disposition.py | 3 +-
> > testenv/Test-c-full.py | 3 +-
> > testenv/Test-cookie-401.py | 3 +-
> > testenv/Test-cookie-domain-mismatch.py | 3 +-
> > testenv/Test-cookie-expires.py | 3 +-
> > testenv/Test-cookie.py | 3 +-
> > testenv/WgetTest.py | 337 -------------------
> > testenv/conf/__init__.py | 49 +++
> > testenv/conf/authentication.py | 9 +
> > testenv/conf/expect_header.py | 7 +
> > testenv/conf/expected_files.py | 42 +++
> > testenv/conf/expected_ret_code.py | 16 +
> > testenv/conf/files_crawled.py | 18 +
> > testenv/conf/hook_sample.py | 15 +
> > testenv/conf/local_files.py | 12 +
> > testenv/conf/reject_header.py | 7 +
> > testenv/conf/response.py | 7 +
> > testenv/conf/rule_sample.py | 10 +
> > testenv/conf/send_header.py | 7 +
> > testenv/conf/server_conf.py | 11 +
> > testenv/conf/server_files.py | 15 +
> > testenv/conf/urls.py | 10 +
> > testenv/conf/wget_commands.py | 10 +
> > testenv/exc/__init__.py | 0
> > testenv/exc/test_failed.py | 7 +
> > testenv/misc/__init__.py | 0
> > testenv/misc/colour_terminal.py | 29 ++
> > testenv/misc/constants.py | 3 +
> > testenv/misc/wget_file.py | 16 +
> > testenv/server/__init__.py | 0
> > testenv/server/ftp/__init__.py | 0
> > testenv/server/ftp/ftp_server.py | 162 +++++++++
> > testenv/server/http/__init__.py | 0
> > testenv/server/http/http_server.py | 467
> > ++++++++++++++++++++++++++
> > testenv/test/__init__.py | 0
> > testenv/test/base_test.py | 223 ++++++++++++
> > testenv/test/http_test.py | 45 +++
> > 58 files changed, 1451 insertions(+), 1035 deletions(-)
> > delete mode 100644 testenv/ColourTerm.py
> > delete mode 100644 testenv/FTPServer.py
> > delete mode 100644 testenv/HTTPServer.py
> > delete mode 100644 testenv/WgetTest.py
> > create mode 100644 testenv/conf/__init__.py
> > create mode 100644 testenv/conf/authentication.py
> > create mode 100644 testenv/conf/expect_header.py
> > create mode 100644 testenv/conf/expected_files.py
> > create mode 100644 testenv/conf/expected_ret_code.py
> > create mode 100644 testenv/conf/files_crawled.py
> > create mode 100644 testenv/conf/hook_sample.py
> > create mode 100644 testenv/conf/local_files.py
> > create mode 100644 testenv/conf/reject_header.py
> > create mode 100644 testenv/conf/response.py
> > create mode 100644 testenv/conf/rule_sample.py
> > create mode 100644 testenv/conf/send_header.py
> > create mode 100644 testenv/conf/server_conf.py
> > create mode 100644 testenv/conf/server_files.py
> > create mode 100644 testenv/conf/urls.py
> > create mode 100644 testenv/conf/wget_commands.py
> > create mode 100644 testenv/exc/__init__.py
> > create mode 100644 testenv/exc/test_failed.py
> > create mode 100644 testenv/misc/__init__.py
> > create mode 100644 testenv/misc/colour_terminal.py
> > create mode 100644 testenv/misc/constants.py
> > create mode 100644 testenv/misc/wget_file.py
> > create mode 100644 testenv/server/__init__.py
> > create mode 100644 testenv/server/ftp/__init__.py
> > create mode 100644 testenv/server/ftp/ftp_server.py
> > create mode 100644 testenv/server/http/__init__.py
> > create mode 100644 testenv/server/http/http_server.py
> > create mode 100644 testenv/test/__init__.py
> > create mode 100644 testenv/test/base_test.py
> > create mode 100644 testenv/test/http_test.py
> >
> > --
> > 1.8.3.2
> >
> > 2014-03-14 21:00 GMT+08:00 Darshit Shah <address@hidden>:
> >
> > On Fri, Mar 14, 2014 at 1:20 PM, 陈子杭 (Zihang Chen) <address@hidden>
> >> wrote:
> >> >
> >> >
> >> >
> >> > 2014-03-14 20:01 GMT+08:00 Darshit Shah <address@hidden>:
> >> >>
> >> >> Hi,
> >> >>
> >> >> Things are getting much cleaner with each iteration. :)
> >> >> I haven't had the time to check the patchset yet. And I may not have
> >> the
> >> >> time until Monday either. However, I do have a few high level
> comments:
> >> >>
> >> >> 1. Please upload all patches directly to the mail. Do not compress
> >> >> them. It may look ugly, but it is much more convenient.
> >> >> This is fixed, if you follow Yousong's recommendations on how to send
> >> >> patches. Using git for it all is always a good idea.
> >> >>
> >> > Yes, I'll follow this advice.
> >> >>
> >> >> 2. Your commit messages are still too long. Always follow 80 chars
> >> >> per line. If you wish to add more, the correct way is to write a
> short
> >> 80
> >> >> char message, leave a blank line and then write however long a commit
> >> >> message you want.
> >> >> The point is, the first line goes into the repository, patch, name,
> >> etc.
> >> >> It
> >> >> is supposed to be an extremely short description of the commit.
> Later,
> >> >> in the body, you can expand as much as you want. Again, I'm not sure
> >> >> what your editor is, but with the git syntax files, ViM highlights
> and
> >> >> lets
> >> >> you know when you've exceeded the maximum line length or are
> >> >> writing on the 2nd line of a commit message, which generally should
> be
> >> >> empty.
> >> >
> >> > I edit ChangeLog, README with vim, if you're talking about them. I
> >> checked
> >> > them with Kate, it surely does exceed 2 or 3 chars in some lines.
> Wired.
> >> > I'll try fixing my .vimrc.
> >> Yes. The same. Vim's textwidth is complex. You probably have a
> wrapmargin
> >> of 2. And that is fine. Your commit messages had multiple sentences,
> which
> >> is hard to read.
> >>
> >> Also, check http://blog.ezyang.com/2010/03/vim-textwidth/ for a better
> >> understanding of the textwidth features.
> >>
> >> >>
> >> >>
> >> >> 3. Also, I'm not a fan of making things very Pythonic. It, in my
> >> personal
> >> >> opinion makes code more unreadable for the general non-Python
> >> >> developer. The whole point of using Python is to ensure that a random
> >> >> C developer can read and follow the code. Hence, I was very
> particular
> >> >> about not using fancy Python features in the Test Suite. The module
> >> >> was horrible, I concede (I'm no Object oriented programmer, I like
> C),
> >> >> but I would prefer the code remains simpler. I'll give more specific
> >> >> examples and pointers to locations which I think should be improved
> >> >> once I look at the code.
> >> >
> >> > I totally understand what you're talking about and I agree with you.
> >> > However,
> >> > I still insist that under certain circumstances, it's better to be a
> >> little
> >> > more
> >> > Pythonic. IMO, C developers who uses this test suite rarely need to
> >> modify
> >> > the framework of the test suite. They only need to write new test case
> >> > scripts, and if needed, write new hook or rule. They can write however
> >> they
> >> > like. I just need make the interface as plain as possible.
> >> >>
> >> Sure. Being pythonic is not always bad. Also, I agree that most of the
> >> times
> >> you do not need to edit the core framework. Especially as long as you
> >> provide
> >> and excellent API. However, oftentimes, you want to know what's going on
> >> in the Test Suite setup, just to see what can be tweaked and how. Else,
> >> you
> >> simply want to know how it runs, and you try to follow the code.
> >>
> >> Specifically, I'd like to stay away from concepts like aliases which can
> >> get
> >> confusing sometimes for someone not used to Python.
> >>
> >> >>
> >> >> On Fri, Mar 14, 2014 at 10:28 AM, 陈子杭 (Zihang Chen) <
> >> address@hidden>
> >> >> wrote:
> >> >> > 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
> >> >> > ---
> >> >> > 此致
> >> >> > 陈子杭
> >> >> > 武汉大学计算机学院
> >> >>
> >> >>
> >> >>
> >> >> --
> >> >> 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
> ---
> 此致
> 陈子杭
> 武汉大学计算机学院
>
--
Thanking You,
Darshit Shah