[Mono-dev] [PATCH] System.Security: trivial fixes for XmlDsig transforms

[Gert Driesen]

Hey Sebastien,

I'm remove all unnecessary changes (like the brace position, bad habits ...) and resubmit the patch for review.

Gert

-----Original Message-----
From: mono-devel-list-bounces at lists.ximian.com [mailto:mono-devel-list-bounces at lists.ximian.com] On Behalf Of Sebastien Pouliot
Sent: donderdag 14 augustus 2008 2:58
To: Gert Driesen
Cc: mono-devel-list at ximian.com
Subject: Re: [Mono-dev] [PATCH] System.Security: trivial fixes for XmlDsig transforms

On Sat, 2008-08-09 at 11:04 +0200, Gert Driesen wrote:
> Hi,
> 
> The attached patch provides some trivial fixes for XmlDsig transforms in
> System.Security, and adds/improves unit tests.
> 
> Let me know if this is ok to commit.

No. It seems there's good stuff (like updating the old syntax of unit
tests) but then there's all the other unneeded stuff part of this patch
(some examples follows).

> 
> 
> 
> 
> 
> differences between files attachment (xmldsig.patch)
> 
> Index: Test/System.Security.Cryptography.Xml/XmlDsigXsltTransformTest.cs
> ===================================================================
> --- Test/System.Security.Cryptography.Xml/XmlDsigXsltTransformTest.cs   (revision 110022)
> +++ Test/System.Security.Cryptography.Xml/XmlDsigXsltTransformTest.cs   (working copy)
> @@ -23,8 +23,17 @@
>  
>         // Note: GetInnerXml is protected in XmlDsigXsltTransform making it
>         // difficult to test properly. This class "open it up" :-)
> -       public class UnprotectedXmlDsigXsltTransform : XmlDsigXsltTransform {
> +       public class UnprotectedXmlDsigXsltTransform : XmlDsigXsltTransform
> +       {

Your interpretation was different than mine so you asked (I think I
suggested it) on mono-dev about this (May 26th) and Miguel's answer (May
27th) was clear: 
        "Same line, there are a couple of samples on that page."

Anyway please do not introduce unneeded changes into your patches.

> +               public UnprotectedXmlDsigXsltTransform ()
> +               {
> +               }
>  
> +               public UnprotectedXmlDsigXsltTransform (bool includeComments)
> +                       : base (includeComments)
> +               {
> +               }
> +
>                 public XmlNodeList UnprotectedGetInnerXml () {
>                         return base.GetInnerXml ();
>                 }
> @@ -36,14 +45,28 @@
>                 protected UnprotectedXmlDsigXsltTransform transform;
>  
>                 [SetUp]
> -               protected void SetUp () 
> +               protected void SetUp ()

This is another case that should not be in the patch (unneeded change).
Yes I too dislike the extra space after () but let's not disturb those
bits on everyone boxes.

...

>  
> -               private XmlDocument GetDoc () 
> +               XmlDocument GetDoc ()

Another example of an unneeded change. private is not required but it's
there (so let it be).

...

> @@ -168,7 +260,7 @@
>  
>                 [Test]
>  #if NET_2_0
> -               [Category ("NotDotNet")]
> +               [Category ("NotDotNet")]

I can't see why this change is needed, so I assume it's about line
endings ?!?

If this is required please let's do the line endings changes in a
separate commit (after ensuring it does not affect unit tests) and
generate the patch afterward.

...


Sebastien

_______________________________________________
Mono-devel-list mailing list
Mono-devel-list at lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list