-
Notifications
You must be signed in to change notification settings - Fork 572
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
#1104: Added real parameterless constructors to all services #1105
Conversation
Thanks @slimCODE. This seems like it'll be useful and doesn't appear to be harmful in any way. The one thing I noticed is that it seems like you've been relatively selective about which services get this treatment. Is there any reason for that? Your patch shows 20 files changed, but there are more like 36 services that are not one of the base classes:
(Also, you might want to add a "Fixes #1104" in your description above just so the issue gets linked somewhere.) |
@brandur-stripe That was a louzy patch, I missed the ones deriving from |
@slimCODE Ah I see what happened. Thanks for the quick fix! Would you mind rebasing to get your two commits squashed down into one? That'll also give us a chance to try the CI build again, which seems to have hit an intermittent network problem and failed. |
7cb5d33
to
19e6215
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, but it looks like you've added extra newlines in a few places. Could you remove these?
@ob-stripe I added those lines to normalize, as there were three lines everywhere else between the ctor (and props) and the "Sync" methods. If you prefer, I'll remove them. |
@slimCODE Ah no, that's fine then. Thanks for going the extra mile :) |
Released in 13.1.0. Thanks again for the contribution! |
This allows these services to serve as a generic type in methods with a
where TService : new()
clause.Fixes #1104.