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

Extension Management API implementation #110

Merged
merged 19 commits into from
Oct 16, 2018
Merged

Extension Management API implementation #110

merged 19 commits into from
Oct 16, 2018

Conversation

GPugnet
Copy link
Contributor

@GPugnet GPugnet commented Oct 12, 2018

PR Summary

This is a basic Extension Management API implementation

PR Checklist

@GPugnet
Copy link
Contributor Author

GPugnet commented Oct 12, 2018

I have a concern regarding the API response. I see on other API already implemented that types are defined, and that the types are applied to the responses. How are defined those types ?

@DarqueWarrior
Copy link
Collaborator

DarqueWarrior commented Oct 15, 2018

The types are created by just reviewing the repsonses from the API. The files are then added and the types applied. If these types are are going to be used in the provider you can skip the type files and just created classes. The classes are then used in the provider.

We add the types because we can then assign a format to them. That way instead of seen Count and value you will get a nicely formatted list or table.

@DarqueWarrior
Copy link
Collaborator

I am going to add this to the provider now.

@DarqueWarrior
Copy link
Collaborator

I am making all the suggested changes. Let me know if you see anything wrong with my suggestions.

)
Process {

if(($PublisherId -and !$ExtensionId) -or (!$PublisherId -and $ExtensionId)){
Copy link
Collaborator

Choose a reason for hiding this comment

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

With both PublisherID and ExtensionID marked as Mandatory above why is this needed?

Write-Output $resp
}
catch {
throw $_
Copy link
Collaborator

Choose a reason for hiding this comment

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

If there is nothing to do with the exception beside re-throw why catch in the first place?

src/extensions.psm1 Show resolved Hide resolved

[switch] $Force
)
if ($Force -or $pscmdlet.ShouldProcess($item, "Remove extension")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

$item needs to be a parameter passed in. Either PublisherID or ExtensionID. I am going to set to ExtensionID

[parameter(Mandatory = $true)]
[string] $ExtensionId,

[parameter(Mandatory = $false)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be mandatory right? What would calling this without this do?

$obj = @{
extensionId = $ExtensionId
publisherId = $PublisherId
installState =
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are missing a @ before the { I will add it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you run this command with -Verbose you will see what I mean.

@DarqueWarrior DarqueWarrior merged commit 62e5f5c into MethodsAndPractices:master Oct 16, 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.

3 participants