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

Call Initialize in third constructor #236

Closed
wants to merge 1 commit into from
Closed

Call Initialize in third constructor #236

wants to merge 1 commit into from

Conversation

mawt
Copy link

@mawt mawt commented Jul 21, 2015

A call to Initialize is missing in the third constructor

/// <summary>
/// Initializes a new instance of the CatFactory class.
/// </summary>
/// <param name='baseUri'>
/// Optional. The base URI of the service.
/// </param>
/// <param name='credentials'>
/// Required. Subscription credentials which uniquely identify client subscription.
/// </param>
/// <param name='handlers'>
/// Optional. The delegating handlers to add to the http client pipeline.
/// </param>
public CatFactory(Uri baseUri, ServiceClientCredentials credentials, params DelegatingHandler[] handlers) : this(handlers)
{
    if (baseUri == null)
    {
        throw new ArgumentNullException("baseUri");
    }
    if (credentials == null)
    {
        throw new ArgumentNullException("credentials");
    }
    this.BaseUri = baseUri;
    this.Credentials = credentials;
    // Missing call to this.Initialize()?
}

Nb. this ought to be tested, as I'm just passing by.

A call to Initialize is missing in the third constructor
@azuresdkci
Copy link

Can one of the admins verify this patch?

@azurecla
Copy link

Hi @mawt, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no contribution license agreement is required at this point. Real humans will now evaluate your PR.

TTYL, AZPRBOT;

@devigned
Copy link
Member

Thank you for the contribution. We'll review it shortly.

@hovsepm
Copy link
Contributor

hovsepm commented Jul 21, 2015

@azuresdkci test this please

@hovsepm
Copy link
Contributor

hovsepm commented Jul 21, 2015

@mawt,
This change is not required since Initialize is called inside this(handlers) constructor.

public CatFactory(Uri baseUri, ServiceClientCredentials credentials, params DelegatingHandler[] handlers) 
: this(handlers)
{
...

@mawt
Copy link
Author

mawt commented Jul 22, 2015

Apologies - I missed that :( Constructor chaining is something I advise against.

@mawt mawt closed this Jul 22, 2015
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.

None yet

5 participants