-
Notifications
You must be signed in to change notification settings - Fork 358
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
[FileVersion] Get-PnPSite change for VersionPolicy #3470
Conversation
@KoenZomers Could you please review and check in this change? Thanks as so much as always :) |
src/Commands/Site/GetSite.cs
Outdated
{ | ||
site.EnsureProperties(s => s.Url, s => s.VersionPolicyForNewLibrariesTemplate, s => s.VersionPolicyForNewLibrariesTemplate.VersionPolicies); | ||
|
||
var s = new PSObject(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be an idea to create a model for this instead of using a freeform PSObject? This is how we do it for the other cmdlets as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KoenZomers Thanks so much for the suggestion! Added a new class for returning version policy object. Could you please check again? The tests results in the PR description is also updated.
src/Commands/Site/GetSite.cs
Outdated
} | ||
} | ||
|
||
public class VersionPolicy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you put it in its own .cs file under src\Commands\Model\SharePoint instead and add some code comments as to what the class does and each of the properties stand for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @KoenZomers for the suggestion! Should I create a new command for my VersionPolicy? Because I saw "[Cmdlet(VerbsCommon.Get, "PnPSiteSensitivityLabel")]" command also get properties from site, it uses a separate command, but not the Get-PnPSite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please. Ideally one cmdlet always return the same type so we can add this at the top:
[OutputType(typeof(Microsoft.SharePoint.Client.Site))]
If it returns anything else, it should have its own cmdlet. Please remember to also write the documentation .md files if you create a new cmdlet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @KoenZomers for the confirmation! Created a new cmdlet, added md file, reverted the changes in Get-PnPSite command and md files. Also updated the test results in the PR description. Could you please review again? Thanks so much!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @KoenZomers , Could you please check the PR again? Thanks a lot!
Excellent @msjennywu, thanks for the changes! |
Before creating a pull request, make sure that you have read the contribution file located at
https://github.com/pnp/powerShell/blob/dev/CONTRIBUTING.md
Type
##Test##