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:[EMAIL PROTECTED] 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: [EMAIL PROTECTED] > [mailto:[EMAIL PROTECTED] 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: [EMAIL PROTECTED] >> [mailto:[EMAIL PROTECTED] 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 <[EMAIL PROTECTED]> 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@lists.ximian.com > http://lists.ximian.com/mailman/listinfo/mono-devel-list > > _______________________________________________ > Mono-devel-list mailing list > Mono-devel-list@lists.ximian.com > http://lists.ximian.com/mailman/listinfo/mono-devel-list > _______________________________________________ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
