Skip to content

net5 compilation and delete obsolete code#179

Merged
alex-kulakov merged 73 commits intoDataObjects-NET:masterfrom
servicetitan:upstream/net5-compilation-and-delete-obsolete-code
Dec 3, 2021
Merged

net5 compilation and delete obsolete code#179
alex-kulakov merged 73 commits intoDataObjects-NET:masterfrom
servicetitan:upstream/net5-compilation-and-delete-obsolete-code

Conversation

@SergeiPavlov
Copy link
Copy Markdown
Contributor

@SergeiPavlov SergeiPavlov commented Oct 21, 2021

SergeiPavlov and others added 18 commits November 17, 2021 14:53
Co-authored-by: Alexey Kulakov <alexey.kulakov@dataobjects.net>
…Type.cs

Co-authored-by: Alexey Kulakov <alexey.kulakov@dataobjects.net>
…SqlType.cs

Co-authored-by: Alexey Kulakov <alexey.kulakov@dataobjects.net>
Co-authored-by: Alexey Kulakov <alexey.kulakov@dataobjects.net>
…vider.cs

Co-authored-by: Alexey Kulakov <alexey.kulakov@dataobjects.net>
Co-authored-by: Alexey Kulakov <alexey.kulakov@dataobjects.net>
Co-authored-by: Alexey Kulakov <alexey.kulakov@dataobjects.net>
@AlexUstinov
Copy link
Copy Markdown
Contributor

AlexUstinov commented Nov 30, 2021

Please take a look at the comments. Also I don't agree with PostgresqlSqlDml rename. The class supposes to contain an extended operations for, in this case, PostgreSQL. Its name is similar to general SqlDml static class which contains variety of different methods that help building SqlExpressions. Thought PostgresqlSqlDml contains only geometric functions it can be extended with more functions. Same thing for PostgresqlSqlFunctionType rename.

I'm sorry, but I didn't notice the similarity with the SqlDml class, but the new name is still similar though. The intention was to provide a meaningful comment to the class and I tried to identify what it actually does. Honestly, we can afford having a dedicated container type for any target DML subdomain like geometry in that case. But I don't mind renaming it back, please just suggest a proper XML comment for the type.

The same reasoning is behind the PostgresqlSqlFunctionType rename. So please suggest an XML comment for that type as well.

@alex-kulakov
Copy link
Copy Markdown
Contributor

alex-kulakov commented Nov 30, 2021

For PostgresqlSqlFunctionType I would suggest

  /// <summary>
  /// Contains additional provider-specific SQL functions.
  /// These functions are not presented in general set of supported SQL
  /// functions and are used only within provider, e.g for operations on
  /// provider-specific types.
  /// </summary>
  public static class PostgresqlSqlFunctionType

PostgresqlSqlDml had summary, probably it could be better, maybe like that

  /// <summary>
  /// A factory for SQL DML operations for Postgresql DBMS.
  /// Similarly to <see cref="Xtensive.Sql.SqlDml"/> it contains
  /// factory methods for provider-specific <see cref="SqlExpression"/>s.
  /// </summary>
  public static class PostgresqlSqlFunctionType

Another type that lacks XML is SqlCustomFunctionType. I suggest following description

  /// <summary>
  /// Defines custom function type.
  /// Used to define functions that are ouside of standard SQL functions
  /// supported by majority of providers. Such functions should be
  /// handleded by the provider within which they are declared.
  /// </summary>
  [Serializable]
  public sealed class SqlCustomFunctionType : IEquatable<SqlCustomFunctionType>

@SergeiPavlov
Copy link
Copy Markdown
Contributor Author

Renamed the classes back and changed the XML comment as suggested

@alex-kulakov alex-kulakov merged commit d739f7f into DataObjects-NET:master Dec 3, 2021
@alex-kulakov
Copy link
Copy Markdown
Contributor

alex-kulakov commented Dec 3, 2021

@AlexUstinov and @SergeiPavlov Tests are failing with NRE with stacks similar to this

KeyFactory.CreateGenericKey(Domain domain, String nodeId, TypeInfo type, TypeReferenceAccuracy accuracy, Tuple tuple, Int32[] keyIndexes) line 150
KeyFactory.Materialize(Domain domain, String nodeId, TypeInfo type, Tuple value, TypeReferenceAccuracy accuracy, Boolean canCache, Int32[] keyIndexes) line 60
ItemMaterializationContext.Materialize(Int32 entityIndex, Int32 typeIdIndex, TypeInfo type, Pair`1[] entityColumns, Tuple tuple) line 51
lambda_method100(Closure , Object[] , Tuple , ItemMaterializationContext )
<>c__DisplayClass2_0`4.<Bind>b__0(T2 arg2, T3 arg3) line 37
ItemMaterializer`1.Materialize(Tuple tuple, MaterializationContext context, ParameterContext parameterContext) line 24
MaterializingReader`1.get_Current() line 46
Enumerable.TryGetFirst[TSource](IEnumerable`1 source, Boolean& found)
Enumerable.First[TSource](IEnumerable`1 source)
QueryResultExtensions.ToScalar[TResult](QueryResult`1& sequence, ResultAccessMethod resultAccessMethod) line 15
TranslatedQuery.ExecuteScalar[TResult](Session session, ParameterContext parameterContext) line 60
QueryProvider.<ExecuteScalar>g__ExecuteScalarQuery|7_0[TResult](TranslatedQuery query, Session session, ParameterContext parameterContext) line 78
QueryProvider.Execute[TResult](Expression expression, Func`4 runQuery) line 107
QueryProvider.ExecuteScalar[TResult](Expression expression) line 81
QueryProvider.Execute[TResult](Expression expression) line 72
Queryable.First[TSource](IQueryable`1 source)
InterfaceTest.MainTest() line 134

Didn't test the changes? And I missed this too so master is broken.

@alex-kulakov
Copy link
Copy Markdown
Contributor

I'm reverting merge.

@SergeiPavlov
Copy link
Copy Markdown
Contributor Author

SergeiPavlov commented Dec 3, 2021

I'm reverting merge.

The NRE is fixed: #202

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants