Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Service dependencies parameter class for RelationalConnection #7494

Closed
wants to merge 1 commit into from

Conversation

ajcvickers
Copy link
Contributor

This is a proof-of-concept implementation of parameter objects for service dependencies. Issue #7465.

Notes:

  • Parameter class is sealed.
  • Ended up going with Clone on the parameters object--cleaner than other things I tried, and general purpose. We can add overloads of Clone as needed when doing point releases without breaking existing code. (May want to consider whether parameter should be options.)
  • Registration is not try-add and is of concrete class

@ajcvickers
Copy link
Contributor Author

cc @anpete @divega @AndriySvyryd

public RelationalConnectionDependencies Clone(
[CanBeNull] IDbContextOptions contextOptions = null,
[CanBeNull] ILogger<IRelationalConnection> logger = null)
=> new RelationalConnectionDependencies(
Copy link
Member

Choose a reason for hiding this comment

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

Consider using a more granular approach: WithOptions, WithLogger, etc.

This way we don't need to obsolete the overloads as we add more dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would we strictly need to obsolete the overloads?

Copy link
Member

Choose a reason for hiding this comment

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

If we don't obsolete them then we'll end up with redundant methods. We could avoid that altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anpete @AndriySvyryd The "With" thing is nice--we've done it before--but it can result in lost of code--as it did before! It somewhat depends how often we think this will happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

@AndriySvyryd That depends on whether we use optional args.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if we use optional args, we will probably need to add new methods to avoid binary breaks. I think I'm leaning towards With methods. We shouldn't have inheritance hierarchies here, like we did with DbInterceptionContext, so the number of methods should be more manageable.

This is a proof-of-concept implementation of parameter objects for service dependencies. Issue #7465.

Notes:
- Parameter class is sealed.
- Ended up going with With methods on the parameters object--cleaner than other things I tried, and general purpose.
- Registration is back to try-add again because apparently Add results in multiple services registered in the ServiceCollection...
@ajcvickers
Copy link
Contributor Author

@anpete @AndriySvyryd @divega Pushed a version using "With..." methods.

@ajcvickers
Copy link
Contributor Author

Merged

@ajcvickers ajcvickers closed this Jan 27, 2017
@bricelam bricelam deleted the BreakMeNot0126 branch February 15, 2017 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants