[Mono-dev] [Fwd: [Mono-patches] r106626 - in trunk/mcs/class/System.Configuration: . System.Configuration Test/System.Configuration Test/standalone]
Atsushi Eno
atsushi at ximian.com
Thu Jun 26 13:58:26 EDT 2008
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
>
More information about the Mono-devel-list
mailing list