It looks good, but three issues:
1. You're not using Evolution, are you? I can't apply these patches
(and, oddly, patch(1) reports that I'm out of disk space when it tries
to apply them -- and I have 11GB free!). You might try attaching a .zip
file with the patches (I could be so lucky that would work, right?).
2. Several of these changes appear to change behavior (e.g.
src/DbLinq/Data/Linq/Mapping/LambdaMetaAccessor.cs returns additional
data), but I don't see any changes to the tests.
Are there tests for these non-logging changes?
For that matter, do all the tests still pass? (I'd check myself, but
(1) makes this difficult w/o manually applying all the changes.)
3. Please pay attention to whitespace (changes, what it's made of,
etc.).
Comments on the patches below...
> Index: src/DbLinq/Data/Linq/Mapping/AttributedMetaModel.cs
> ===================================================================
> --- src/DbLinq/Data/Linq/Mapping/AttributedMetaModel.cs (revision 1239)
> +++ src/DbLinq/Data/Linq/Mapping/AttributedMetaModel.cs (working copy)
> @@ -204,9 +204,13 @@
>
> MetaTable metaTable;
> if (_Tables.TryGetValue(tableType, out metaTable))
> - yield return metaTable;
> + yield return metaTable;
Whitespace only change. Sure, this removes tabs with spaces, but it
also increases the patch size and increases my mental overhead. :-)
> else
> - yield return AddTableType(tableType);
> + {
> + metaTable = AddTableType(tableType);
> + if (metaTable != null)
> + yield return metaTable;
> + }
This looks like a semantic change that I worry about. It used to
(potentially) return null. Now it won't. Does this still pass all
tests? Do we need a new test to check this behavior?
> Index: src/DbLinq/Data/Linq/Mapping/LambdaMetaAccessor.cs
> ===================================================================
> --- src/DbLinq/Data/Linq/Mapping/LambdaMetaAccessor.cs (revision 1239)
> +++ src/DbLinq/Data/Linq/Mapping/LambdaMetaAccessor.cs (working copy)
> @@ -14,18 +14,28 @@
> //This will go away with C# 4.0 ActionExpression
> static Delegate MakeSetter(MemberInfo member, Type
> memberType, Type declaringType)
> {
> + if (member == null)
> + throw new ArgumentNullException("member");
> +
> Type delegateType =
> typeof(Action<,>).MakeGenericType(declaringType, memberType);
>
> switch (member.MemberType)
> {
> case MemberTypes.Property: {
> - MethodInfo method =
> ((PropertyInfo)member).GetSetMethod();
> + var property = (PropertyInfo)member;
'property' seems improperly indented here. (The line contains mixed
tabs and spaces; please use only spaces.)
> + MethodInfo method = property.GetSetMethod();
> if (method != null)
> return Delegate.CreateDelegate(delegateType, method);
> var ca =
> member.GetCustomAttributes(typeof(ColumnAttribute), true)[0] as
> ColumnAttribute;
> member = declaringType.GetField(ca.Storage,
> BindingFlags.NonPublic | BindingFlags.Public |
> BindingFlags.Instance | BindingFlags.Static);
> - goto case MemberTypes.Field;
> + if (member == null)
> + {
> + throw new ApplicationException(string.Format("Failed
> to find field '{0}' on type '{1}', which was"
> + + " specified as the Storage field of property
> '{2}'.", ca.Storage, declaringType.FullName, property.Name));
> + }
> +
> + goto case MemberTypes.Field;
Ditto the indentation comments here.
> Index: src/DbLinq/Data/Linq/Sugar/Implementation/SqlBuilder.cs
> ===================================================================
> --- src/DbLinq/Data/Linq/Sugar/Implementation/SqlBuilder.cs (revision
> 1239)
> +++ src/DbLinq/Data/Linq/Sugar/Implementation/SqlBuilder.cs (working copy)
> @@ -164,11 +164,14 @@
> var literalOperands = new List<SqlStatement>();
> foreach (var operand in operands)
> {
> - var operandPrecedence =
> ExpressionQualifier.GetPrecedence(operand);
> - var literalOperand = BuildExpression(operand, queryContext);
> - if (operandPrecedence > currentPrecedence)
> - literalOperand =
> sqlProvider.GetParenthesis(literalOperand);
> - literalOperands.Add(literalOperand);
> + if (operand != null)
> + {
> + var operandPrecedence =
> ExpressionQualifier.GetPrecedence(operand);
> + var literalOperand = BuildExpression(operand,
> queryContext);
> + if (operandPrecedence > currentPrecedence)
> + literalOperand =
> sqlProvider.GetParenthesis(literalOperand);
> + literalOperands.Add(literalOperand);
> + }
Instead of re-indenting this entire block, I would instead prefer
foreach (var operand in operands)
{
if (operand == null)
continue;
// as before
}
It makes the patch smaller (nice), and it also minimizes indentation
(doubly nice, unless you like to see blocks starting at column 80...).
I also wonder if this is the right place to fix things; changing the
calling code so that it doesn't return 'null's may be preferable. (I
know when making other fixes that's the route I took, removing 'null's.)
> Index: src/DbLinq/Util/TextWriterExtension.cs
> ===================================================================
> --- src/DbLinq/Util/TextWriterExtension.cs (revision 1239)
> +++ src/DbLinq/Util/TextWriterExtension.cs (working copy)
> @@ -50,6 +50,7 @@
> // we just ignore NREs
> catch (NullReferenceException)
> {
> + // TODO: fix the NREs properly and remove this, so
> expression trees in the log are complete.
> }
> }
>
> @@ -166,7 +167,7 @@
> var lines = new List<string>
> {
> WriteHeader(expression, header, depth++),
> - WriteLiteral(expression.Constructor.Name,
> "Ctor", depth)
> +
> WriteLiteral(System.Reflection.ConstructorInfo.ConstructorName, "Ctor", depth)
> };
I'm not sure what this does, exactly. Can you further explain what the
intent is?
Thanks,
- Jon
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"DbLinq" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/dblinq?hl=en
-~----------~----~----~----~------~----~------~--~---
