[Mono-dev] [PATCH] Validation for <xsl:output> attributes.

[Atsushi Eno]

Gert Driesen wrote:
>  
> 
>> -----Original Message-----
>> From: mono-devel-list-bounces at lists.ximian.com 
>> [mailto:mono-devel-list-bounces at lists.ximian.com] On Behalf 
>> Of Atsushi Eno
>> Sent: vrijdag 23 december 2005 19:29
>> To: Gert Driesen
>> Cc: mono-devel-list at lists.ximian.com
>> Subject: RE: [Mono-dev] [PATCH] Validation for <xsl:output> 
>> attributes.
>>
>>
>>>> -----Original Message-----
>>>> From: Atsushi Eno [mailto:atsushi at ximian.com] 
>>>> Sent: vrijdag 23 december 2005 18:54
>>>> To: Gert Driesen
>>>> Cc: mono-devel-list at ximian.com
>>>> Subject: RE: [Mono-dev] [PATCH] Validation for <xsl:output> 
>>>> attributes.
>>>>
>>>>> Comments inline 
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: mono-devel-list-bounces at lists.ximian.com 
>>>>>> [mailto:mono-devel-list-bounces at lists.ximian.com] On Behalf 
>>>>>> Of Atsushi Eno
>>>>>> Sent: dinsdag 20 december 2005 6:26
>>>>>> To: Gert Driesen
>>>>>> Cc: mono-devel-list at ximian.com
>>>>>> Subject: Re: [Mono-dev] [PATCH] Validation for <xsl:output> 
>>>>>> attributes.
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>> The attached patch implements validation for <xsl:output> 
>>>>>> attributes, and
>>>>>>> adds unit tests.
>>>>>> Thanks!
>>>>>>
>>>>>>> I've also added some unit tests for XsltCompileException 
>>>>>> and XslException.
>>>>>>> Some test are marked NotWorking, due to bugs in Mono (for 
>>>>>> which I'll report
>>>>>>> bug reports later).
>>>>>>>
>>>>>> Some comments:
>>>>>>
>>>>>> 	- You can try Mainsoft XSLT standalone tests. Go to
>>>>>> 	  Test/System.Xml.Xsl/standalone and run "make 
>> run-test", then
>>>>>> 	  you can find some regressions.
>>>>>> 	- Your code that checks attributes is good.
>>>>>> 	- "indent" in xsl:output is "yes" by default 
>> when the output
>>>>>> 	  method is "html", unlike when it is "xml" 
>> ("no"). That's why
>>>>>> 	  we have string value instead of boolean in 
>> XslOutput class.
>>>>> I now use an enum for this internally, which allows us to 
>>>> continue exposing
>>>>> Intend as a bool.
>>>>>
>>>>>> 	- unindent cases in switches, i.e.
>>>>>>
>>>>>> 		switch (foo) {
>>>>>> 		case bar:
>>>>>> 			...
>>>>> Done.
>>>>>
>>>>>> 	- The reason why you marked [NotWorking] on
>>>>>> 	  XsltExceptionTests.Constructor2() is because
>>>>>>
>>>>>> 		xsltException = new XsltException 
>> ((string) null,cause);
>>>>>> 		Assert.AreEqual (string.Empty, 
>> xsltException.Message);
>>>>>> 	  "fails", right? Hmm, It's still okay to keep 
>> this test, but
>>>>>> 	  I don't think it is kind of thing we should 
>> fix. Having empty
>>>>>> 	  message for an exception does not make sense.
>>>>>>
>>>>>> 	  I guess, most of the reason in NotWorking are 
>> like that. If
>>>>>> 	  so, you don't have to file bugs for them. 
>> Just add some
>>>>>> 	  comments in the sources.
>>>>> I've fixes these "bugs", and all tests now pass.
>>>> Lemme say again, "it is not kind of thing we should fix". 
>> After your
>>>> fix,
>>>> no one can understand what is going on from the Message property.
>>> If you actually specify a zero-length or null message, then 
>> this not really
>>> that odd behaviour. No ?
>> No. Your test reports like
>>
>> expected <null>
>> but actually <"XSLT compile error ...">
>>
>> that is, it expects not to explain that it is XSLT compile error, but
>> not to tell
>> anything. Why on earth it could be better?
> 
> I'm not saying that an empty (or null) message is better. But I'm saying
> that it makes sense that if you construct an exception with a null or
> zero-length message, you actually get a zero-length message.

You are saying the same idea with different words.

>> BTW it should not be the reason you could change code in 
>> advance without
>> further discussion. 
> 
> All I did was, instead of reporting a bug report, I fixed the "issue"
> myself. I got the impression that you did not consider it to be an important
> bug (and I agree that its not), and that you wanted me to file a bug report
> as a reminder (for when you'd have time to fix it). I thought I'd help you
> by "fixing" it myself ... But well, guess not :(

You wrote a lot of helpful tests here and there, but it's not. Also this
"bug" is usually not worthy of spending much words. You committed 
changes without further discussion, which is a bad sign.

Atsushi Eno