On Sat, 2005-07-02 at 00:45, kfogel@collab.net wrote:
> Vivek <vivek@collab.net> writes:
> > [[[
> > Create a new file for testing date related issues.
> >
> > *subversion/tests/clients/cmdline/date_tests.py: New File.
> > ]]]
>
> Don't forget the space after the "*".
Ouch. :-)
> Isn't this a revised version of an earlier patch? If so, put "v2" or
> "v3" or whatever's appropriate in the Subject line.
Yes, keeping track is sometimes difficult. :-)
> Copyright should just say "2005", not "2000-2004".
Ok.
> If 't' were only a helper variable used in creating 'time_array'
> below, I wouldn't object to having such a cryptic name for a top-level
> variable. But it is also used in some of the test functions. In that
> case, please give it a more comprehensible name, like 'local_time' or
> something.
Will do.
>
> This is the only test that uses time_array, therefore shouldn't
> time_array be a local variable within the test?
This is the only test _currently_ in the file which uses the time_array
variable. Someday, someone might add something to this file which uses
the variable and then he would have to either make a local copy or make
it global.
Why not do that now?
> On Windows, 'svn log' ends lines with CRLF instead of just LF. Does
> your comparison above still work then (and if so, why? :-) ) ?
Wait, don't shoot me! I never test under Windows before sending patches.
:-)
>
> By the way, note that r15191 changed the default content of the test
> files. A/mu would now have "This is the file 'mu'.\n" (the newline
> wasn't there before r15191).
Thanks for pointing this out. Will see to it.
> > +#----------------------------------------------------------------------
> > +# Date is stored as an unversiond property in the reop. If we try
> Fitz already corrected the "reop" ==> "repos" typo in
Ouch!!
>
> This is interesting, I don't think we've had any automated hook
> testing yet, but this strategy seems to work. Does it pass for you on
> both Unix and Win32 systems?
See my confession above. :-)
> Again, as Fitz also asked, why the svntest.main.file_append() when
> open().write() would do? I know you swiped it from another test; but
> IMHO just because one test does it doesn't mean we have to perpetuate
> it :-). This is about writing, not appending, so let's just do the
> most straightforward thing.
Ok. Point taken.
>
> I believe you can pass 'None' instead of '[]', and if so, that would
> be a better flag value.
Ok.
>
> Same about [] vs None.
Here too.
>
> > + e_out, err = svntest.main.run_svn (1, 'cat', '-r',
> > + '{' + str(arg) + '}', mu_path)
> > + if err.pop() != 'svn: Bogus date\n':
> > + raise svntest.Failure
>
> What does "e_out" signify, how is it different from "out"?
Just another variable. Note that variable out is used later. To prevent
out from being overwritten I introduced a new variable.
>
> I recommend against the .pop() idiom. If someone went to examine err
> later, it wouldn't have the same data. Instead, just index into err[].
Ok.
>
> >
> Nice test, I like the strategy!
Hey, thanks!
>
> That comment seems a little out of date. I mean, you've got the hook
> testing working for win32 as well as Posix...
>
Oh yes, will change that.
> See, here the log message differs from the one in the body of your
> message -- here it has the space after the asterisk. If there were
> only one copy, I'd know which one to review :-).
Umm, I lost. :-)
Thanks for the review Karl.
Expect V4(?) very soon.
Vivek
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jul 2 10:30:31 2005