[prev in list] [next in list] [prev in thread] [next in thread]
List: velocity-dev
Subject: Re: Patch
From: "Will Glass-Husain" <wglass () forio ! com>
Date: 2005-09-23 17:43:25
Message-ID: 008801c5c066$4f7c71c0$927ba8c0 () Apollo
[Download RAW message or body]
Thanks - appreciate the explanation. I threw the patches in our issue
tracker. I should have time to review it tonight or tomorrow. At first
glance they look almost perfect.
Not to beat a dead horse, but using JIRA is more than busy work. It really
does make life easier for the committers, and it only takes a minute to
create an issue.
* JIRA keeps issues visible. Although I'm trying hard to be responsive,
sometimes we can't get to everything at once. (I have another patch in
progress on my code base, and a day job to boot). Patches can get lost in
the noise on the dev list.
* If there's any problems or comments on the patch, JIRA provides a forum
for people to review and comment.
* And it's still going to be a few months till a 1.5 release. If another
user hits the same bug - JIRA should be one of the first places they look.
Thanks again for contributing. I'm really eager to produce a 1.5 release
this Fall. The biggest priority right now is bug fixing, so this is great.
Best,
WILL
----- Original Message -----
From: "Llewellyn Falco" <isidore@setgame.com>
To: "Velocity Developers List" <velocity-dev@jakarta.apache.org>
Sent: Friday, September 23, 2005 12:47 AM
Subject: Re: Patch
> Sorry,
>
> apparently i am being a bit confusing.
>
> the patch was rather large (six lines of code) so i can understand why
> i should have explained it more clearly,
>
> The patch is against 3 issues, 1 of them i submitted when the old
> bugzilla, the other 2 don't exist. but there is a unit test per each issue
> submitted with the patch.
> don't quite understand why i would create an issue merely to close it.
> i mean if i needed to show my boss i was working the busy work would make
> sense, but as we are all doing this
> in our free time.... if you feel you need notes that better explain the
> patch than the unit tests, by all means fell free, but i personally can't
> think what i would write that is more precise than actual code.
>
> issue 1 (this is Velocity-381)
> the issue is the is shown by the following test, as in both should
> return ""
> ---
> public void testMethod() throws Exception {
> assertEquals("", parseString("$!main.toString()", this));
> }
>
> public void testField() throws Exception {
> assertEquals("", parseString("$!main", this));
> }
> public String toString() {
> return null;
> }
> ----
> the line of code change is
> - if (value == null)
> + if (value == null || value.toString() == null)
>
>
> issue 2 (no jira issue)
> the info is created wrong.
> code change
> ----
> - method = rsvc.getUberspect().getMethod(o, methodName,
> params, new Info("",1,1));
> + method = rsvc.getUberspect().getMethod(o, methodName,
> params, new Info(context.getCurrentTemplateName(), getLine(),
> getColumn()));
> -----
> the test checks against an uberspect that throws exceptions with it can't
> find stuff.
> (this is also useful for development, but i am trying to start with small
> changes as to keep things simple, so kept it to testing)
> ---
> public Info getInfoFor(String velocity) throws Exception {
> try {
> parseString(velocity, this);
> fail("Uberspect Should have thrown an exception");
> throw new Error("Shouldn't be able to reach this point");
> } catch (VelocityParsingError t) {
> return t.getInfo();
> }
> }
>
> public void testInfoForField() throws Exception {
> Info i = getInfoFor("$main.unknownField");
> assertInfoEqual(i, "$main.unknownField", 1, 7);
> }
>
> public void testInfoForMethod() throws Exception {
> Info i = getInfoFor("$main.unknownMethod()");
> assertInfoEqual(i, "$main.unknownMethod()", 1, 7);
> }
>
> private void assertInfoEqual(Info i, String name, int line, int column)
> {
> assertEquals("Template Name", name, i.getTemplateName());
> assertEquals("Template Line", line, i.getLine());
> assertEquals("Template Column", column, i.getColumn());
> }
> ---
>
> The third test also deals with the uberspect. design indicates that the
> uberspect should be asked to find the method, and therefore if you wanted
> a to write an uberspect to check when you are calling methods on null's
> you could write one. but the parser blocked it.
> the test is
> ---
> public void testNullPointer() {
> assertErrorThrown("$main.getNull().callMethod()");
> }
>
> private void assertErrorThrown(String string) {
> try {
> String result = parseString(string, this);
> Assert.fail("parsing '" + string + "' did not fail but returned '"
> + result + "'");
> } catch (Throwable t) {
> return;
> }
> }
> ----
>
> the code change is the removal of premature exit
> - if (result == null)
> - {
> - return null;
> - }
>
>
>
> >Really appreciate your contribution - I don't mean to throw up
> unnecessary obstacles.
> thank you, i look forward to contributing in the future
>
> llewellyn.
>
>
> ----- Original Message -----
> From: "Will Glass-Husain" <wglass@forio.com>
> To: "Velocity Developers List" <velocity-dev@jakarta.apache.org>;
> "Llewellyn Falco" <isidore@setgame.com>
> Sent: Thursday, September 22, 2005 11:01 PM
> Subject: Re: Patch
>
>
>> Hi Llewllyn,
>>
>> I'm a bit confused. The patch that you are submitting - does it solve
>> the
>> issue in Velocity-381? If so, please attach to that issue.
>>
>> Does the patch solve a different problem? Then create a new issue in
>> JIRA,
>> describe the problem, attach the test, and attach the solution.
>>
>> I'm eager to review, comment on, (and if they are ready) apply your
>> patches,
>> but for ease of processing it's best they go through JIRA. (discussion
>> can
>> be on the dev list or on the JIRA issue). Let me know if you've any
>> difficulties logging on or using the system.
>>
>> Thanks again,
>> WILL
>>
>> ----- Original Message -----
>> From: "Llewellyn Falco" <isidore@setgame.com>
>> To: "Velocity Developers List" <velocity-dev@jakarta.apache.org>
>> Sent: Thursday, September 22, 2005 9:14 PM
>> Subject: Re: Patch
>>
>>
>>> ok,
>>>
>>> you can reproduce all the bug by running the attached tests without
>>> the
>>> patches
>>> (as is the point of unit tests)
>>>
>>>
>>> The patches fix them, most of them are in the order of 1 line, so
>>> easier to see the code rather than explain.
>>>
>>> the only issue i saw that related was.
>>>
>>> velocity-381
>>>
>>> again each test is pretty straight forward about what it is testing.
>>>
>>> llewellyn.
>>>
>>> ----- Original Message -----
>>> From: "Will Glass-Husain" <wglass@forio.com>
>>> To: "Velocity Developers List" <velocity-dev@jakarta.apache.org>;
>>> "Llewellyn Falco" <isidore@setgame.com>
>>> Sent: Thursday, September 22, 2005 10:29 AM
>>> Subject: Re: Patch
>>>
>>>
>>>> Can you please create a JIRA issue then?
>>>>
>>>> Please list exactly the problem you were experiencing, including how to
>>>> reproduce it. Then attach the patch and state how it solves the
>>>> problem.
>>>>
>>>> Really appreciate your contribution - I don't mean to throw up
>>>> unnecessary obstacles. But it makes a big difference in ease of
>>>> tracking
>>>> to have every change tracked and submitted through our issue tracker.
>>>> Among other benefits, it documents the change for those who experience
>>>> the same bug in an earlier version, and it provides a place for other
>>>> developers (e.g. me) to ask questions and offer suggestions for
>>>> improving
>>>> the specific patch. (and the list serv does not always include all
>>>> attached files).
>>>>
>>>> Thanks again, WILL
>>>>
>>>> ----- Original Message -----
>>>> From: "Llewellyn Falco" <isidore@setgame.com>
>>>> To: "Velocity Developers List" <velocity-dev@jakarta.apache.org>
>>>> Sent: Thursday, September 22, 2005 10:11 AM
>>>> Subject: Re: Patch
>>>>
>>>>
>>>>>I don't know if there are any jira issue's involved.
>>>>>
>>>>> It solves the issues of i was experiencing of..
>>>>>
>>>>> null not being able to pass to uberspect to resolve, as is indecated
>>>>> by
>>>>> the uberspect implementation and api.
>>>>>
>>>>> info not being correctly created for a failure in a method call.
>>>>>
>>>>> if an object is not null, but the toString returns null the silent
>>>>> failed.
>>>>>
>>>>>
>>>>> the ant tests passed.
>>>>>
>>>>> Llewellyn.
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
>>>>> For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
>>>>>
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
>>>> For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
>>> For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
>> For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic