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

review docs #153

Merged
merged 6 commits into from
Dec 13, 2018
Merged

Conversation

AnthonySteele
Copy link
Contributor

Summarise the changes this Pull Request makes.

review of the Readme.md document. Needed for a 4.0 release

Please include a reference to a GitHub issue if appropriate.

related to #130

@AnthonySteele AnthonySteele requested a review from a team as a code owner December 13, 2018 09:26
@martincostello martincostello added this to the v4.0.0 milestone Dec 13, 2018
@AnthonySteele
Copy link
Contributor Author

AnthonySteele commented Dec 13, 2018

There is a todo in the docs, but I think that this might be also a code issue.
StatsDServiceCollectionExtensions.AddStatsD does not have a way to specify the new SocketProtocol or an entire StatsDConfiguration object.

Some of the mechanism in there such as ResolveStatsDTransport might also be not needed any more.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@martincostello
Copy link
Member

You would use this overload to provide an entire config where the IServiceProvider is available.

An overload that specified the StatsDConfiguration as an input would require separate pre-bootstrap to get values from config as otherwise you're trying to configure it before DI registration is complete. If you really wanted to do that yourself, there's nothing to stop your registering StatsDConfiguration that way with the service collection yourself.

@AnthonySteele
Copy link
Contributor Author

AnthonySteele commented Dec 13, 2018

Indeed, given that overload you can do

        public static IServiceCollection AddStatsD(this IServiceCollection services, StatsDConfiguration configuration)
        {
            return services.AddStatsD((s) => configuration);
        }

But as you pointed out that's not as useful, since you more likely want to populate the StatsDConfiguration with values from config etc, and so create it in a func to call after the DI container is up and working.

it's IMHO worth reviewing that code again, but as a separate thing; it is out of scope of this docs review.

@martincostello
Copy link
Member

I'll look at doing a pass on the /// docs later today.

@AnthonySteele AnthonySteele merged commit 2c989e9 into justeattakeaway:master Dec 13, 2018
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