[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 11:40:54 EDT 2008
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.
> Modified: trunk/mcs/class/System.Configuration/System.Configuration/ClientConfigurationSystem.cs
> ===================================================================
> --- trunk/mcs/class/System.Configuration/System.Configuration/ClientConfigurationSystem.cs 2008-06-26 09:57:29 UTC (rev 106625)
> +++ trunk/mcs/class/System.Configuration/System.Configuration/ClientConfigurationSystem.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.
> @@ -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.
> Modified: trunk/mcs/class/System.Configuration/System.Configuration/ConfigurationElement.cs
> ===================================================================
> --- trunk/mcs/class/System.Configuration/System.Configuration/ConfigurationElement.cs 2008-06-26 09:57:29 UTC (rev 106625)
> +++ trunk/mcs/class/System.Configuration/System.Configuration/ConfigurationElement.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.
> @@ -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).
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) ?
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.
Atsushi Eno
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.
>
More information about the Mono-devel-list
mailing list