Skip to content

Conversation

@shaneer
Copy link
Collaborator

@shaneer shaneer commented Sep 23, 2018

No description provided.

@shaneer shaneer requested a review from ysa23 September 23, 2018 01:48
Copy link
Contributor

@ysa23 ysa23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an integration test that makes sure you get the right parameters upon query

{
public readonly string Query;
public readonly string[] Parameters;
public readonly double MiliSeconds;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be long not double


namespace OhioBox.Moranbernate.Logging
{
public static class QueryLoggingRepo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class should be internal

{
public static void Initialize(Assembly[] assemblies, IQueryLogger[] queryLoggers)
{
MappingRepoDictionary.InitializeAssemblies(assemblies);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets upgrade the major version - deprecating MappingRepoDictionary.InitializeAssemblies as public method and replace it with the initializer

{
public class MoranbernateInitializer
{
public static void Initialize(Assembly[] assemblies, IQueryLogger[] queryLoggers)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since query loggers are optional, give the parameter a default value

{
public class MoranbernateInitializer
{
public static void Initialize(Assembly[] assemblies, IQueryLogger[] queryLoggers)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want, you can make it fluent:
new MoranbernateInitializer()
.AddAssemblies(...)
.AddLogger(...)


namespace OhioBox.Moranbernate.Logging
{
public static class QueryLoggingRepo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to QueryLogger

public interface IQueryLogger
{
void LogFailedQuery(QueryPerformanceLog queryLog, Exception exception);
void LogSuccesfulQuery(QueryPerformanceLog queryLog);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo!!! OMG!!! :)

public class QueryPerformanceLog
{
public readonly string Query;
public readonly string[] Parameters;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an array of IDbParameter - so you don't need to do any work to get and also it will give more freedom to the logger to do whatever it needs

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.

2 participants