Re: [Bug-wget] [GSoC] Refactoring the Test Suite
Thank you both, Darshit and Giuseppe, for paying attention and guiding me about the patch series. I'm very grateful. 2014-06-07 22:44 GMT+08:00 Giuseppe Scrivano gscriv...@gnu.org: Darshit Shah dar...@gmail.com writes: With Zihang's copyright assignment process over, is there anything that is still blocking the process of merging this set of patches? Nothing really, I am going to push this series soon. Zihang, thanks for your contribution! Regards, Giuseppe -- Regards, Chen Zihang, Computer School of Wuhan University --- 此致 陈子杭 武汉大学计算机学院
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
Darshit Shah dar...@gmail.com writes: With Zihang's copyright assignment process over, is there anything that is still blocking the process of merging this set of patches? Nothing really, I am going to push this series soon. Zihang, thanks for your contribution! Regards, Giuseppe
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
With Zihang's copyright assignment process over, is there anything that is still blocking the process of merging this set of patches? On Tue, Apr 15, 2014 at 11:33 AM, 陈子杭 (Zihang Chen) chsc4...@gmail.com wrote: Hi all. I here upload v3 of the patch series. Changes: * Fix no new line endings, which eliminates patch10 of v2 * README is updated along the patches, which eliminates patch9 of v2 * patch11 of v2 is merged into patch5 of v3 * Fix various inappropriateness metions in previous mail Speaking about the overhead of loading conf package (which has a lot of small scripts), I timed `make check` and the real time of the original test suite is 15.129s and that of patch-applied test suite is 16.786s, on first sight the original is faster (about 2s). I still prefer small scripts, mainly because of their extensibility and maintainability. The reason why the latter test suite is slower, I think, is that `make check` create a new Python process for every test case (which does not take advantage of Python's import mechanism), and that's not what modern test suites do. I'd suggest that we make a bootstrap Python script, which makes sure that only one Python process is created during test. And make `make check` use that script to do the tests. Thanks in advance and cheers. Zihang Chen (8): 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 testenv/ChangeLog | 143 testenv/ColourTerm.py | 23 -- testenv/FTPServer.py | 162 - testenv/HTTPServer.py | 467 -- testenv/README| 77 +++-- 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 | 47 +++ 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 | 1 + testenv/exc/test_failed.py| 7 + testenv/misc/__init__.py | 1 + testenv/misc/colour_terminal.py | 31 ++ testenv/misc/wget_file.py | 16 + testenv/server/__init__.py| 1 + testenv/server/ftp/__init__.py| 1 + testenv/server/ftp/ftp_server.py | 162 + testenv/server/http/__init__.py | 1 + testenv/server/http/http_server.py| 467 ++ testenv/test/__init__.py | 1 + testenv/test/base_test.py | 226 +
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
2014-04-08 7:24 GMT+08:00 Darshit Shah dar...@gmail.com:
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.
My fault, sorry. PyCharm (the IDE I use) sometimes does something kinky
silently, especially when it optimizes the imports automatically.
2. The colour_teminal.py file you create has no newline at the end. This
was pointed out earlier too if I remember correctly.
Yes. My fault too. I thought PyCharm has taken care of it for me.
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.
Sure.
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
Of course they can be in one file. However I was afraid that the file would
end up too large. And what if there are more rules to be added? Maybe I
overthought it.
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.
Well, I have to point out that when these scripts are imported, Python
actually compile them into byte codes and loads the byte codes into memory.
So the overhead of importing is not that much.
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]
I admit that I overthought this after you pointing it out.
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?
My fault three.
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.
Reasonable. I'll do that.
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?
My fault four.
I have to admit that I was a bit lazy when doing the patches, which leads
to so many stupid mistakes. Sorry for wasting your time and thanks for
pointing these out. I will try to overcome my laziness and do my patches
more carefully :)
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.
Absolutely.
[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 000..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) chsc4...@gmail.comwrote:
Thanks very much in advance for paying attention to this newbie patch
series!
2014-04-01 15:39 GMT+08:00 陈子杭 (Zihang Chen) chsc4...@gmail.com:
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
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
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 000..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) chsc4...@gmail.comwrote:
Thanks very much in advance for paying attention to this newbie patch
series!
2014-04-01 15:39 GMT+08:00 陈子杭 (Zihang Chen) chsc4...@gmail.com:
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 +-
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
Thanks very much in advance for paying attention to this newbie patch series! 2014-04-01 15:39 GMT+08:00 陈子杭 (Zihang Chen) chsc4...@gmail.com: 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
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
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. - 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. - 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). 2. The newly formatted error string will not run, since expected_ret_code is a int, and it will not format with %s. 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? 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. 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. - Double blank lines. - 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. 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. 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#6 [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.
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
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#6
[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
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
2014-03-14 20:01 GMT+08:00 Darshit Shah dar...@gmail.com:
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.
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.
On Fri, Mar 14, 2014 at 10:28 AM, 陈子杭 (Zihang Chen) chsc4...@gmail.com
wrote:
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
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
On Fri, Mar 14, 2014 at 1:20 PM, 陈子杭 (Zihang Chen) chsc4...@gmail.com wrote: 2014-03-14 20:01 GMT+08:00 Darshit Shah dar...@gmail.com: 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) chsc4...@gmail.com wrote: 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
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
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
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
Hi Yousong, So sorry about the line endings, I'll have to do a thorough check. BTW, the pyc files in 0001.patch was deleted in the second commit. 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 --- 此致 陈子杭 武汉大学计算机学院
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
On Mon, Mar 10, 2014 at 8:46 AM, 陈子杭 (Zihang Chen) chsc4...@gmail.comwrote: 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. 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. 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. 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
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
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
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
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.comwrote: 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 --- 此致 陈子杭 武汉大学计算机学院
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
Thanks, I'll have a try. 2014-03-09 16:06 GMT+08:00 Yousong Zhou yszhou4t...@gmail.com: On 9 March 2014 11:38, 陈子杭 (Zihang Chen) chsc4...@gmail.com wrote: Yes, you're right. I mostly work under Windows. Didn't notice that. My bad. BTW is there anything similar to guideline for setting the development environment? Please let me know if there is one. You can find many with keywords like git, line ending, git rebase, interactive, git format-patch, git send-email. But I suggest using a Linux system for this task. I used to install a Debian server as a virtual machine which needed only 64MB of host memory, then I sshed into it. Almost all tools like vim, git, etc. should just work out of box. yousong -- Regards, Chen Zihang, Computer School of Wuhan University --- 此致 陈子杭 武汉大学计算机学院
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
Hi Zihang, I just had a brief glance through the whole commit. That's a very large change! It's essentially the same code with lots of moving around and cosmetic changes. However, I do have a couple of issues with it: 1. I found it really difficult to follow the code. You should edit the README file to reflect the current scenario and how should a developer follow it. 2. It seems like you've created some really nice abstractions, it would very nice to explain them so the developers for Wget know what to look at and what to edit. 3. While the code surely is more pythonic, it creates a slight problem. It's *more* pythonic. Most people who have to deal with this code are not users who use Python everyday. I think, a little lesser of strict Python syntax and a little more of simpler syntax will allow non-Python developers to more easily follow the code. The point of using Python to rewrite the old test suite was that Perl was a bit too cryptic and people had to spend too much time understanding the code first before they could edit it. I don't want to repeat that with having truly pythonic code which takes more time to follow for a C developer. Others, please chime in on this. I like the overall restructuring though. And if the abstractions do work the way I think they do, I believe this could be a good idea. I'll look at it in much more detail when I get the time. On Sun, Mar 9, 2014 at 2:22 AM, Darshit Shah dar...@gmail.com wrote: Hi, Thanks for the refactoring. However, you've included makefile and makefile.in which are autogenerated files and should not be commited. Also, your patch has trailing whitespace errors. And I don't think you've added an entry to ChangeLog either. Please look into these. I haven't seen your patch yet. The Makefile errors mean I can't apply it without a lot of extra work. On Sat, Mar 8, 2014 at 5:38 PM, 陈子杭 (Zihang Chen) chsc4...@gmail.comwrote: Hi. Here's a patch refactoring CommonMethods, HTTPTest and project hierarchy. 19 tests passed, 1 xfailed as expected. Let me know if there's any problem. -- Regards, Chen Zihang, Computer School of Wuhan University --- 此致 陈子杭 武汉大学计算机学院 -- Thanking You, Darshit Shah -- Thanking You, Darshit Shah
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
Hi, Zihang and Darshit, and all. On 9 March 2014 09:39, Darshit Shah dar...@gmail.com wrote: Hi Zihang, I just had a brief glance through the whole commit. That's a very large change! It's essentially the same code with lots of moving around and cosmetic changes. However, I do have a couple of issues with it: 1. I found it really difficult to follow the code. You should edit the README file to reflect the current scenario and how should a developer follow it. 2. It seems like you've created some really nice abstractions, it would very nice to explain them so the developers for Wget know what to look at and what to edit. Hi, Zihang. The patch is really big as a single commit. You'd better split it into multiple small ones each for a single purpose, without breaking the code with each commit if possible. That way we can refer to and comment on the code more easily. 3. While the code surely is more pythonic, it creates a slight problem. It's *more* pythonic. Most people who have to deal with this code are not users who use Python everyday. I think, a little lesser of strict Python syntax and a little more of simpler syntax will allow non-Python developers to more easily follow the code. The point of using Python to rewrite the old test suite was that Perl was a bit too cryptic and people had to spend too much time understanding the code first before they could edit it. I don't want to repeat that with having truly pythonic code which takes more time to follow for a C developer. To be honest, I am fine with the current Perl implementation. My last several patches for the Perl-based test framework are my first try with Perl. It does not take me much time to understand the design and modify a few lines of code. I think documentation or self-explaining code is the solution. Others, please chime in on this. I like the overall restructuring though. And if the abstractions do work the way I think they do, I believe this could be a good idea. I'll look at it in much more detail when I get the time. On Sun, Mar 9, 2014 at 2:22 AM, Darshit Shah dar...@gmail.com wrote: Hi, Thanks for the refactoring. However, you've included makefile and makefile.in which are autogenerated files and should not be commited. Also, your patch has trailing whitespace errors. And I don't think you've added an entry to ChangeLog either. Please look into these. I haven't seen your patch yet. The Makefile errors mean I can't apply it without a lot of extra work. Hi Zihang, looks like you development environment has to be configured right for this. The newline character in your code in now '\r\n' which should be '\n'. There are mode changes from 100755 to 100644 in the git commit, which is not right. Those .py files should retain their executable attributes. yousong
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
2014-03-09 9:39 GMT+08:00 Darshit Shah dar...@gmail.com: Hi Zihang, I just had a brief glance through the whole commit. That's a very large change! It's essentially the same code with lots of moving around and cosmetic changes. However, I do have a couple of issues with it: 1. I found it really difficult to follow the code. You should edit the README file to reflect the current scenario and how should a developer follow it. 2. It seems like you've created some really nice abstractions, it would very nice to explain them so the developers for Wget know what to look at and what to edit. 3. While the code surely is more pythonic, it creates a slight problem. It's *more* pythonic. Most people who have to deal with this code are not users who use Python everyday. I think, a little lesser of strict Python syntax and a little more of simpler syntax will allow non-Python developers to more easily follow the code. The point of using Python to rewrite the old test suite was that Perl was a bit too cryptic and people had to spend too much time understanding the code first before they could edit it. I don't want to repeat that with having truly pythonic code which takes more time to follow for a C developer. I'm most sorry for the lack of documentation. I'll try adding some to README, CHANGELOG and the code itself (though I think some of the code is self-explaining itself, I'll add comments for non-pythoners, sorry again.) Others, please chime in on this. I like the overall restructuring though. And if the abstractions do work the way I think they do, I believe this could be a good idea. I'll look at it in much more detail when I get the time. On Sun, Mar 9, 2014 at 2:22 AM, Darshit Shah dar...@gmail.com wrote: Hi, Thanks for the refactoring. However, you've included makefile and makefile.in which are autogenerated files and should not be commited. Also, your patch has trailing whitespace errors. And I don't think you've added an entry to ChangeLog either. Please look into these. I haven't seen your patch yet. The Makefile errors mean I can't apply it without a lot of extra work. On Sat, Mar 8, 2014 at 5:38 PM, 陈子杭 (Zihang Chen) chsc4...@gmail.comwrote: Hi. Here's a patch refactoring CommonMethods, HTTPTest and project hierarchy. 19 tests passed, 1 xfailed as expected. Let me know if there's any problem. -- Regards, Chen Zihang, Computer School of Wuhan University --- 此致 陈子杭 武汉大学计算机学院 -- Thanking You, Darshit Shah -- Thanking You, Darshit Shah -- Regards, Chen Zihang, Computer School of Wuhan University --- 此致 陈子杭 武汉大学计算机学院
Re: [Bug-wget] [GSoC] Refactoring the Test Suite
2014-03-09 11:31 GMT+08:00 Yousong Zhou yszhou4t...@gmail.com: Hi, Zihang and Darshit, and all. On 9 March 2014 09:39, Darshit Shah dar...@gmail.com wrote: Hi Zihang, I just had a brief glance through the whole commit. That's a very large change! It's essentially the same code with lots of moving around and cosmetic changes. However, I do have a couple of issues with it: 1. I found it really difficult to follow the code. You should edit the README file to reflect the current scenario and how should a developer follow it. 2. It seems like you've created some really nice abstractions, it would very nice to explain them so the developers for Wget know what to look at and what to edit. Hi, Zihang. The patch is really big as a single commit. You'd better split it into multiple small ones each for a single purpose, without breaking the code with each commit if possible. That way we can refer to and comment on the code more easily. 3. While the code surely is more pythonic, it creates a slight problem. It's *more* pythonic. Most people who have to deal with this code are not users who use Python everyday. I think, a little lesser of strict Python syntax and a little more of simpler syntax will allow non-Python developers to more easily follow the code. The point of using Python to rewrite the old test suite was that Perl was a bit too cryptic and people had to spend too much time understanding the code first before they could edit it. I don't want to repeat that with having truly pythonic code which takes more time to follow for a C developer. To be honest, I am fine with the current Perl implementation. My last several patches for the Perl-based test framework are my first try with Perl. It does not take me much time to understand the design and modify a few lines of code. I think documentation or self-explaining code is the solution. Others, please chime in on this. I like the overall restructuring though. And if the abstractions do work the way I think they do, I believe this could be a good idea. I'll look at it in much more detail when I get the time. On Sun, Mar 9, 2014 at 2:22 AM, Darshit Shah dar...@gmail.com wrote: Hi, Thanks for the refactoring. However, you've included makefile and makefile.in which are autogenerated files and should not be commited. Also, your patch has trailing whitespace errors. And I don't think you've added an entry to ChangeLog either. Please look into these. I haven't seen your patch yet. The Makefile errors mean I can't apply it without a lot of extra work. Hi Zihang, looks like you development environment has to be configured right for this. The newline character in your code in now '\r\n' which should be '\n'. Yes, you're right. I mostly work under Windows. Didn't notice that. My bad. BTW is there anything similar to guideline for setting the development environment? Please let me know if there is one. Thanks, Yousong. There are mode changes from 100755 to 100644 in the git commit, which is not right. Those .py files should retain their executable attributes. yousong -- Regards, Chen Zihang, Computer School of Wuhan University --- 此致 陈子杭 武汉大学计算机学院
