-
Notifications
You must be signed in to change notification settings - Fork 326
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
New Command: pp website webfile list #6415
base: main
Are you sure you want to change the base?
Conversation
Thanks, we'll try to review it ASAP! |
@ktskumar I added the |
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 @ktskumar, I reviewed your PR. Could you look at my comments? Thanks in advance!
); | ||
} | ||
|
||
#initValidators(): void { |
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.
Let's move to using zod here instead, just like you did for the weblinks Pr.
); | ||
} | ||
|
||
private async getWebSiteId(dynamicsApiUrl: string, args: CommandArgs): Promise<any> { |
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.
Let's move private functions to below the commandAction function. Could we also type this to string instead of any?
also:
private async getWebSiteId(dynamicsApiUrl: string, args: CommandArgs): Promise<any> { | |
private async getWebsiteId(dynamicsApiUrl: string, args: CommandArgs): Promise<string> { |
if (args.options.websiteId) { | ||
return args.options.websiteId; | ||
} | ||
const options: PpWebSiteOptions = { |
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.
This options object is not really used, let's remove it.
responseType: 'json' | ||
}; | ||
|
||
if (options.name) { |
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.
Let's remove this if statement because we know for certain args.options.websiteName should be filled here. Let's use args.options.websiteName!
if necessary.
private async getWebSiteId(dynamicsApiUrl: string, args: CommandArgs): Promise<any> { | ||
if (args.options.websiteId) { | ||
return args.options.websiteId; | ||
} |
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.
Let's add a verbose logging line below here, about retrieving the website Id
Closes #6262
Add new command for listing webfiles from Powerpages website