Identity project noUnusedParameters, noImplicitReturns and noUnusedLocals tsconfig fixes#11453
Conversation
mohsin-mehmood
requested review from
bterlson,
daviwil,
sadasant,
schaabs,
sophiajt and
xirzec
as code owners
ghost
added
Cosmos
Azure.Identity
customer-reported
|
Thank you for your contribution mohsin-mehmood! We will review the pull request and get back to you soon. |
mohsin-mehmood
changed the title
| * @param resource The resource to use when getting the token | ||
| */ | ||
| protected async getAzureCliAccessToken(resource: string) { | ||
| protected async getAzureCliAccessToken(resource: string): Promise<unknown> { |
There was a problem hiding this comment.
Any reason to choose unknown here instead of say inlining the type of the object containing the stdout, stderr and error?
There was a problem hiding this comment.
Currently in identity.api.md the return type of this method is Promise<unknown> and I was not sure if we should change the public API. Initially, I thought of setting it to Promise<any>
There was a problem hiding this comment.
I believe changing from unknown to a strongly typed value especially in the output of a method should not be a breaking change. Doing so from any would be breaking though.
cc @xirzec to confirm
There was a problem hiding this comment.
Marking this conversation as unresolved to ensure we have input from @xirzec here
There was a problem hiding this comment.
In-lining the type is better than unknown and any, though a small type alias or interface would be even better. I'm good with the linked commit though.
In this particular case, I wonder how much we have to worry about breaking changes for protected members. It seems like the only reason this method is protected is out of a misguided strategy for testing. We should be using something like sinon to mock out getAzureCliAccessToken instead of inheritance.
If @jonathandturner is willing, perhaps we could make this private unless we really do anticipate developers to replace this implementation in their live apps?
| private persistenceEnabled: boolean; | ||
| private redirectUri: string; | ||
| private authorityHost: string; | ||
| private authenticationRecord: AuthenticationRecord | undefined; |
There was a problem hiding this comment.
@jonathandturner This was added in #10994
Was curious as to how you envisioned this being used as it is currently unused
There was a problem hiding this comment.
@mohsin-mehmood Looks like the latest changes in the master branch do make use of the authenticationRecord. Can you pull in the changes from master and update the PR?
ramya-rao-a
left a comment
ramya-rao-a
left a comment
There was a problem hiding this comment.
Looks good to me, thanks @mohsin-mehmood!
@sadasant, Please take another look and merge the PR if you have no concerns
4 participants
PR for #11438 #11439 and #11440