[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
Thu Jun 26 13:47:10 EDT 2008
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
More information about the Mono-devel-list
mailing list