Skip to content

Addition of comment based help #152

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

Merged
merged 1 commit into from
Oct 21, 2023

Conversation

Celerium
Copy link
Contributor

@Celerium Celerium commented Dec 4, 2022

Addresses issue #56

Quite a bit to go over with this initial update so ZERO rush to get this in place but overall 99% of the changes are the addition of comment-based help to all wrapper functions with support for the Get-Help <command name> -online function.

Example:
Get-Help Get-ITGlueUsers -online

I believe I have everything in a good initial state but with writing some much documentation it wouldn't surprise me if there are grammar or consistency issues between the wrappers. I have tested most of the functions after the modifications and I have not found any issues yet but I am only familiar with GET methods right now.

What are everyone's thoughts on the structure and or next steps?

  • Is there a better way to relay\show JSON examples in the help comments?

Unless otherwise defined all modifications are additions of comment-based help

  1. Pester Tests
  • Updated to Pester 5
  • General test improvements (could use some more tweaks)
  • Added HelpComment.Tests to help validate help-based comments
  1. APIKey
  • Added cmdletbinding to all functions
  • Fixed basic scriptAnalyzer warnings
  • New Test-ITGlueAPIKey
    • Added because its helpful when needing to validate general functionality or when using RMM deployment tools
  1. BaseURI
  • Added cmdletbinding to all functions
  • Fixed basic scriptAnalyzer warnings
  1. ModuleSettings
  • Added cmdletbinding to all functions
  • Fixed basic scriptAnalyzer warnings
  • New Remove-ITGlueModuleSettings
    • Nice to remove saved settings from systems when no longer needed
  1. ConfigurationInterfaces
  • Added cmdletbinding to New-ITGlueConfigurationInterfaces
  1. Configurations
  • Added cmdletbinding to New-ITGlueConfigurations
  1. ConfigurationStatues
  • Added cmdletbinding to New-ITGlueConfigurationStatuses
  • Added cmdletbinding to Set-ITGlueConfigurationStatuses
  1. ConfigurationTypes
  • Added cmdletbinding to New-ITGlueConfigurationTypes
  • Added cmdletbinding to Set-ITGlueConfigurationTypes
  1. Contacts
  • Added cmdletbinding to New-ITGlueContacts
  1. ContactTypes
  • Added cmdletbinding to New-ITGlueContactTypes
  • Added cmdletbinding to Set-ITGlueContactTypes
  1. FlexibleAssetFields
  • Added cmdletbinding to New-ITGlueFlexibleAssetFields
  • Might need to add in mandatory parameters to Get-ITGlueFlexibleAssetFields (Can come later)
  1. FlexibleAssetTypes
  • Added cmdletbinding to New-ITGlueFlexibleAssetTypes
  • Added cmdletbinding to Set-ITGlueFlexibleAssetTypes
  1. FlexibleAssets
  • Might need to add mandatory parameter's to Get-ITGlueFlexibleAssets (Can come later)
  1. Locations
  • Added cmdletbinding to New-ITGlueLocations
  • Should the "Remove-ITGlueLocations" data param need an update paramSet?
  1. Manufacturers
  • Added cmdletbinding to New-ITGlueManufacturers
  • Added cmdletbinding to Set-ITGlueManufacturers
  1. Models
  • Added cmdletbinding to New-ITGlueModels
  1. Organizations
  • Added cmdletbinding to New-ITGlueOrganizations
  1. OrganizationStatuses
  • Added cmdletbinding to New-ITGlueOrganizationStatuses
  • Added cmdletbinding to Set-ITGlueOrganizationStatuses
  1. OrganizationTypes
  • Added cmdletbinding to New-ITGlueOrganizationTypes
  • Added cmdletbinding to Set-ITGlueOrganizationTypes
  1. PasswordCategories
  • Added cmdletbinding to New-ITGluePasswordCategories
  • Added cmdletbinding to Set-ITGluePasswordCategories
  1. Passwords
  • Added cmdletbinding to New-ITGluePasswords
  1. Users
  • Added cmdletbinding to Set-ITGlueUsers

Copy link
Contributor

@CalebAlbers CalebAlbers left a comment

Choose a reason for hiding this comment

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

Phenomenal work, @Celerium! This undoubtedly took a ton of time. I haven't gone through the entire PR yet, but a few things spring to mind:

with support for the Get-Help -online function.

Get-Help Get-ITGlueUsers -online
This will take you to https://api.itglue.com/developer/#accounts-users

I think this is a fantastic add - definitely something I wish I had when originally using the module 🙂

What are everyone's thoughts on the structure and or next steps?

I left a few comments, but the biggest thing that calls out to me would be considering inlining the parameter descriptions. I'd love to have some dialogue on that with you.

Is there a better way to relay\show JSON examples in the help comments?
I'm not sure that there really is a great option there. This goes into a potentially larger discussion on how to best create the PSObjects that these functions use (maybe a generator of some kind, or use of object types?), but I think leaving the documentation in the state it is in currently is a great option, and we can build on it over time.

@Celerium
Copy link
Contributor Author

Celerium commented Dec 8, 2022

With all the new development going on when would be a good time to merge these changes? I was thinking once #157 is completed.

This would give me or others time to review any grammar or consistency issues with help descriptions between the functions as well as give a good jumping-off point for further module development.

@@ -107,13 +107,15 @@
FunctionsToExport = 'Add-ITGlueAPIKey',
'Get-ITGlueAPIKey',
'Remove-ITGlueAPIKey',
'Test-ITGlueAPIKey',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once #157 is complete and if this is something that has value to add to the module I can migrate the Invoke section to the new internal API call function(s).

@Celerium
Copy link
Contributor Author

I've updated various, grammar, spelling, and word consistency for each of the functions and feel like this is a good starting point for the modules about_help.

I can switch this to ready for review so it can be merged now or after #157.

@Celerium Celerium marked this pull request as ready for review December 18, 2022 21:15
@adrianwells
Copy link
Collaborator

@Celerium this is an impressive amount of work as @CalebAlbers noted. It seems it should be merged soon. @davidhaymond thoughts?

@Celerium
Copy link
Contributor Author

@Celerium this is an impressive amount of work as @CalebAlbers noted. It seems it should be merged soon. @davidhaymond thoughts?

@adrianwells thank you, something to note as it's been a while, so it would be a good idea to do a once-over of this addition as well as make sure any of the new functions include comment-based help.

@davidhaymond
Copy link
Collaborator

@Celerium this is an impressive amount of work as @CalebAlbers noted. It seems it should be merged soon. @davidhaymond thoughts?

I would love to see this merged and included in v2.3.0. I'll see if I can find time this week to review it.

Copy link
Collaborator

@davidhaymond davidhaymond left a comment

Choose a reason for hiding this comment

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

I've done a quick review and made a few suggestions. Amazing work, @Celerium!

@davidhaymond
Copy link
Collaborator

@Celerium This PR seems ready to me. Could you rebase your branch on top of the current master branch and squash all the commits? I'd like to try to keep the Git history relatively clean.

It looks like you've allowed maintainers to edit the PR, so if you like, I can rebase and merge for you.

@Celerium
Copy link
Contributor Author

@Celerium This PR seems ready to me. Could you rebase your branch on top of the current master branch and squash all the commits? I'd like to try to keep the Git history relatively clean.

It looks like you've allowed maintainers to edit the PR, so if you like, I can rebase and merge for you.

@davidhaymond, if you could that would be wonderful. Thank you

@davidhaymond davidhaymond merged commit 1c060be into itglue:master Oct 21, 2023
@davidhaymond
Copy link
Collaborator

I've cleaned up this PR and merged into master! Thanks again for your incredible contribution, @Celerium!

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.

4 participants