[Mono-dev] [Fwd: [Mono-patches] r106626 - in trunk/mcs/class/System.Configuration: . System.Configuration Test/System.Configuration Test/standalone]

Gert Driesen gert.driesen at telenet.be
Fri Jun 27 04:13:17 EDT 2008


Atsushi,

This is not making much sense. Just about every member/class in
System.Xml.Serialization is marked like that.
Developers using that (eg. for our web services stack) must then indeed be
real idiots.
 
Marking a class or member as ".NET Framework infrastructure" means you
should avoid using it in user code as its behavior is not documented and
subject to changes. However, it's of course perfectly valid to use those
classes from within the ".NET/Mono Framework infrastructure".

In this specific case we're talking about an interface that by itself does
not have any logic. But should the usage of that interface related to
ConfigurationErrorsException change, than my unit tests will allow us to
notice that.

With my change, my goal was not just compatibility with MS. The only reason
why - in this case - compatibility was nice to have, was to get unit tests
that pass on both Mono and MS.

I can't see why implementing a public interface is worse than using an
internal interface in combination with checks for a specific class for the
exact same purpose?

Gert

-----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 27 juni 2008 2:37
To: 'mono-devel-list'
Subject: Re: [Mono-dev] [Fwd: [Mono-patches] r106626 - in
trunk/mcs/class/System.Configuration: . System.Configuration
Test/System.Configuration Test/standalone]

Actually anyone who uses this IConfigErrorInfo is an idiot because
it is explicitly written as .NET FX internal.

"This API supports the .NET Framework infrastructure and is not
intended to be used directly from your code."

So, compatibility is COMPLETELY no value here.

You don't have to flood mono-dev next time. I will silently
eliminate extraneous changes. I really don't care about those
pointless compatibility and will be glad to eliminate.

Atsushi Eno


Gert Driesen wrote:
> Atsushi,
> 
> I'm not deliberately ignoring your or anyone else's advice. Perhaps my
idea
> of trivial changes is different than yours, definitely if a very large
part
> of those changes is covered by unit tests. Next time, I'll flood mono-dev
> with small patches ;-)
> 
> I'm definitely not claiming my changes are more important than performance
> improvements, but I'd be surprised if both can't be combined.
> 
> Gert
> 
> -----Original Message-----
> From: Atsushi Eno [mailto:atsushi at ximian.com] 
> Sent: donderdag 26 juni 2008 19:58
> To: Gert Driesen
> Cc: 'mono-devel-list'
> Subject: Re: [Mono-dev] [Fwd: [Mono-patches] r106626 - in
> trunk/mcs/class/System.Configuration: . System.Configuration
> Test/System.Configuration Test/standalone]
> 
> In short, you are not going to change your commit policy right?
> 
> When I go ahead and make significant refactoring, I'll clean your
> changes up and mark insignificant compatibility tests as Ignore.
> 
> Insignificant compatibility mastur* should not block significant
> performance improvements.
> 
> Atsushi Eno
> 
> 
> Gert Driesen wrote:
>> Hey Atsushi,
>>
>> Apart from the change in ClientConfigurationSystem my patch was about
> using
>> the IConfigErrorInfo interface for getting filename/linenumber info
> instead
>> of explicitly checking for XmlTextReader, or using internal
IConfigXmlNode
>> interface.
>>
>> Before the introduction of IConfigErrorInfo, we had to resort to this. My
>> changes just allow us to use this new interface, which - at the same time
> -
>> allows us to write unit tests for this (as this now matches the MS
>> implementation).
>>
>> I don't intend to write a book about these trivial changes, but I've
>> responded to your comments inline.
>>
>> Gert
>>
>> -----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: donderdag 26 juni 2008 17:41
>> To: Gert Driesen
>> Cc: 'mono-devel-list'
>> Subject: Re: [Mono-dev] [Fwd: [Mono-patches] r106626 - in
>> trunk/mcs/class/System.Configuration: . System.Configuration
>> Test/System.Configuration Test/standalone]
>>
>> No. You claim as if your change were only about one issue, which it
>> NOT true. Here is concrete description:
>>
>>> Modified:
>
trunk/mcs/class/System.Configuration/System.Configuration/AppSettingsSection
>> .cs
>>> ===================================================================
>>> ---
>
trunk/mcs/class/System.Configuration/System.Configuration/AppSettingsSection
>> .cs	2008-06-26 09:57:29 UTC (rev 106625)
>>> +++
>
trunk/mcs/class/System.Configuration/System.Configuration/AppSettingsSection
>> .cs	2008-06-26 10:31:07 UTC (rev 106626)
>>> @@ -73,7 +73,7 @@
>>>  			if (File != "") {
>>>  				try {
>>>  					Stream s = System.IO.File.OpenRead
>> (File);
>>> -					XmlReader subreader = new
>> XmlTextReader (s);
>>> +					XmlReader subreader = new
>> ConfigXmlTextReader (s, File);
>>>  					base.DeserializeElement (subreader,
>> serializeCollectionKey);
>>>  					s.Close ();
>>>  				}
>> For example it is about ConfigXmlTextReader change.
>>
>> => The use of ConfigXmlTextReader is necessary as it implements
>> IConfigErrorInfo. I consider this part of the same change.
>>
>>> Modified:
>
trunk/mcs/class/System.Configuration/System.Configuration/ClientConfiguratio
>> nSystem.cs
>>> ===================================================================
>>> ---
>
trunk/mcs/class/System.Configuration/System.Configuration/ClientConfiguratio
>> nSystem.cs	2008-06-26 09:57:29 UTC (rev 106625)
>>> +++
>
trunk/mcs/class/System.Configuration/System.Configuration/ClientConfiguratio
>> nSystem.cs	2008-06-26 10:31:07 UTC (rev 106626)
>>> @@ -32,28 +32,39 @@
>>>  using System.Reflection;
>>>  using System.Configuration.Internal;
>>>  
>>> -namespace System.Configuration {
>>> -
>>> +namespace System.Configuration
>>> +{
>>>  	internal class ClientConfigurationSystem : IInternalConfigSystem
>>>  	{
>>> -		readonly Configuration cfg;
>>> +		Configuration cfg;
>>>  
>>> -		public ClientConfigurationSystem () {
>>> -			Assembly a = Assembly.GetEntryAssembly();
>>> -			string exePath =
>> AppDomain.CurrentDomain.SetupInformation.ConfigurationFile;
>>> -            
>>> -			if (a == null && exePath == null)
>>> -				exePath = "";
>>> -            
>>> -			cfg =
>> ConfigurationManager.OpenExeConfigurationInternal
>> (ConfigurationUserLevel.None, a, exePath);
>>> +		public ClientConfigurationSystem ()
>>> +		{
>>>  		}
>>>  
>>> +		private Configuration Configuration {
>>> +			get {
>>> +				if (cfg == null) {
>>> +					Assembly a =
>> Assembly.GetEntryAssembly();
>>> +					string exePath =
>> AppDomain.CurrentDomain.SetupInformation.ConfigurationFile;
>>> +
>>> +					if (a == null && exePath == null)
>>> +						exePath = string.Empty;
>>> +
>>> +					try {
>>> +						cfg =
>> ConfigurationManager.OpenExeConfigurationInternal (
>>> +
>> ConfigurationUserLevel.None, a, exePath);
>>> +					} catch (Exception ex) {
>>> +						throw new
>> ConfigurationErrorsException ("Error Initializing the configuration
>> system.", ex);
>>> +					}
>>> +				}
>>> +				return cfg;
>>> +			}
>>> +		}
>>> +
>>>  		object IInternalConfigSystem.GetSection (string configKey)
>>>  		{
>>> -			if (cfg == null)
>>> -				return null;
>>> -
>>> -			ConfigurationSection s = cfg.GetSection (configKey);
>>> +			ConfigurationSection s = Configuration.GetSection
>> (configKey);
>>>  			return s != null ? s.GetRuntimeObject () : null;
>>>  		}
>>>  
>> This is about lazy initialization. There is no bug fixes involved.
>>
>> => As I mentioned earlier, this is not related to the IConfigErrorInfo
>> implementation. However, it does fix a bug. Standalone test t28 failed
> with
>> a TypeInitializationException before this fix. 
>>
>>> @@ -460,17 +459,12 @@
>>>  				return false;
>>>  			}
>>>  
>>> -			try {
>>> -				reader = new XmlTextReader (stream);
>>> +			using (XmlTextReader reader = new
>> ConfigXmlTextReader (stream, streamName)) {
>>>  				ReadConfigFile (reader, streamName);
>>> -			} finally {
>>> -				if (reader != null)
>>> -					reader.Close();
>>>  			}
>>>  			return true;
>>>  		}
>>>  
>>> -
>>>  		void ReadConfigFile (XmlTextReader reader, string fileName)
>>>  		{
>>>  			reader.MoveToContent ();
>> Insignificant change.
>>
>> => Same as above. The use of ConfigXmlTextReader is necessary as it
>> implements IConfigErrorInfo. Also used Disposable pattern (unrelated, big
>> deal).
>>  
>>> Modified:
>
trunk/mcs/class/System.Configuration/System.Configuration/ConfigurationEleme
>> nt.cs
>>> ===================================================================
>>> ---
>
trunk/mcs/class/System.Configuration/System.Configuration/ConfigurationEleme
>> nt.cs	2008-06-26 09:57:29 UTC (rev 106625)
>>> +++
>
trunk/mcs/class/System.Configuration/System.Configuration/ConfigurationEleme
>> nt.cs	2008-06-26 10:31:07 UTC (rev 106626)
>>> @@ -322,13 +322,13 @@
>>>  					} else if (this is
>> ConfigurationSection && reader.LocalName == "configSource") {
>>>  						/* ignore */
>>>  					} else if
>> (!OnDeserializeUnrecognizedAttribute (reader.LocalName, reader.Value))
>>> -						throw new
>> ConfigurationException ("Unrecognized attribute '" + reader.LocalName +
>> "'.");
>>> +						throw new
>> ConfigurationErrorsException ("Unrecognized attribute '" +
> reader.LocalName
>> + "'.", reader);
>>>  
>>>  					continue;
>>>  				}
>>>  				
>>>  				if (readProps.ContainsKey (prop))
>>> -					throw new ConfigurationException
>> ("The attribute '" + prop.Name + "' may only appear once in this
> element.");
>>> +					throw new
>> ConfigurationErrorsException ("The attribute '" + prop.Name + "' may only
>> appear once in this element.", reader);
>>>  
>>>  				string value = null;
>>>  				try {
>> Different bugfix.
>>
>> => I agree, but it's all about the same issue. The real bugfix is about
>> implementing support for IConfigErrorInfo, and this specific change just
>> allows us to use that implementation of the XmlTextReader for adding
>> filename/linenumber info to exception messages.
>>
>>> @@ -141,34 +143,34 @@
>>>  		//
>>>  		public static string GetFilename (XmlReader reader)
>>>  		{
>>> -			if (reader is XmlTextReader)
>>> -				return ((XmlTextReader)reader).BaseURI;
>>> +			if (reader is IConfigErrorInfo)
>>> +				return ((IConfigErrorInfo) reader).Filename;
>>>  			else
>>> -				return String.Empty;
>>> +				return null;
>>>  		}
>>>  
>>>  		public static int GetLineNumber (XmlReader reader)
>>>  		{
>>> -			if (reader is XmlTextReader)
>>> -				return ((XmlTextReader)reader).LineNumber;
>>> +			if (reader is IConfigErrorInfo)
>>> +				return ((IConfigErrorInfo)
>> reader).LineNumber;
>>>  			else
>>>  				return 0;
>>>  		}
>>>  
>>>  		public static string GetFilename (XmlNode node)
>>>  		{
>>> -			if (!(node is IConfigXmlNode))
>>> -				return String.Empty;
>>> +			if (!(node is IConfigErrorInfo))
>>> +				return null;
>>>  
>>> -			return ((IConfigXmlNode) node).Filename;
>>> +			return ((IConfigErrorInfo) node).Filename;
>>>  		}
>>>  
>>>  		public static int GetLineNumber (XmlNode node)
>>>  		{
>>> -			if (!(node is IConfigXmlNode))
>>> +			if (!(node is IConfigErrorInfo))
>>>  				return 0;
>>>  
>>> -			return ((IConfigXmlNode) node).LineNumber;
>>> +			return ((IConfigErrorInfo) node).LineNumber;
>>>  		}
>>>  		
>>>  		public override void GetObjectData (SerializationInfo info,
>> StreamingContext context)
>>> @@ -178,9 +180,8 @@
>>>  			info.AddValue ("ConfigurationErrors_Line", line);
>>>  		}
>>>  
>>> -		string bareMessage = "";
>>> -		string filename = "";
>>> -		int line = 0;
>>> +		readonly string filename;
>>> +		readonly int line;
>>>  	}
>>>  #pragma warning restore
>>>  }
>> Insignificant or doubtful changes (I needed to read the actual patch
>> again after quick check on ChangeLog).
>>
>> => There's nothing doubtful about these changes. These are fully covered
> by
>> unit tests (that now pass on MS and Mono).
>>
>> The changes are almost niche as it could have been done as
>> using XmlTextReader.BaseURI (the only difference between BaseURI and
>> this Filename thingy is whether it is an absolute URL or a file name,
>> isn't it) ?
>>
>> => More or less, yes. But my changes are about using the IConfigErrorInfo
>> interface were applicable, instead of relying on the internal
> IConfigXmlNode
>> interface, or explicitly checking for XmlTextReader.
>>
>> => My changes actually make it a little easier for your
> XmlNodeReader-based
>> implementation, as you don't have to check for explicit checks against
>> XmlTextReader and just need to implement IConfigErrorInfo in your
>> XmlNodeReader class for getting filename/linenumber info in exception
>> messages.
>>
>> I keep somewhat special eyes on your changes because you usually
>> seem to make larger changes than usual hackers do. And this time
>> "unfortunately" we were actually discussing System.Configuration
>> refactoring. That's why your change is specially mentioned.
>>
>> => I have no problem with people monitoring my changes. I tend to do that
>> myself, and this is part of why I like open-source development.
>>
>> If you feel strong about reverting my changes, please go ahead. If you
> want
>> me to update your patch to support IConfigErrorInfo, just let me know.
>>
>> Gert
>>
>> Gert Driesen wrote:
>>> Hey Jb,
>>>
>>> Sorry dude, but this wasn't a large changeset.
>>>
>>> If the definition of a large changeset is a patch which adds a large set
>> of
>>> unit tests, then I'm guilty as charged.
>>>
>>> Apart from removing a few extra tabs (my mistake), everything I changed
> is
>>> documented in the changelog and covered by unit tests or standalone
> tests.
>>> There's only one part of the patch that could be committed separately,
> and
>>> this is the change to ClientConfigurationSystem. And again, this change
>>> fixes a failing standalone test (t28).
>>>
>>> Please be reasonable. What more can you ask?
>>>
>>> Gert
>>>
>>> -----Original Message-----
>>> From: mono-devel-list-bounces at lists.ximian.com
>>> [mailto:mono-devel-list-bounces at lists.ximian.com] On Behalf Of Jb Evain
>>> Sent: donderdag 26 juni 2008 15:11
>>> To: Gert Driesen
>>> Cc: Atsushi Eno; mono-devel-list
>>> Subject: Re: [Mono-dev] [Fwd: [Mono-patches] r106626 - in
>>> trunk/mcs/class/System.Configuration: . System.Configuration
>>> Test/System.Configuration Test/standalone]
>>>
>>> Hey,
>>>
>>> On 6/26/08, Gert Driesen <gert.driesen at telenet.be> wrote:
>>>>  > Even only with that point, I'm pretty much tempted to revert your
>>>>  > changes.
>>>>
>>>>
>>>> Yeah, I'm glad my (any?) contributions are that much appreciated.
>>> Come on Gert, it's definitely not the first time that you're told that
>>> your commits are:
>>>
>>> * totally not atomic,
>>> * mixing totally different concerns,
>>>
>>> And for some of us that keep an eye on the code coming in, it makes
>>> that task much harder.
>>>
>>> That doesn't mean that we don't appreciate contributions. But once
>>> again, I already told you that I was not happy with the way you're
>>> making commits, and am not the first one. And I can certainly
>>> understand the maintainer frustration to see this huge changeset
>>> coming in.
>>>
>> _______________________________________________
>> Mono-devel-list mailing list
>> Mono-devel-list at lists.ximian.com
>> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>>
>> _______________________________________________
>> Mono-devel-list mailing list
>> Mono-devel-list at lists.ximian.com
>> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>>
> 
> 
> 

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



More information about the Mono-devel-list mailing list