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

Adds a new command spp model list Closes #6103 #6172

Closed
wants to merge 1 commit into from

Conversation

mkm17
Copy link
Contributor

@mkm17 mkm17 commented Jul 22, 2024

Adds a new command spp model list

Closes #6103

@milanholemans
Copy link
Contributor

Thank you @mkm17! We'll review it anytime soon!

@mkm17 mkm17 force-pushed the issues/6103_spp_model_list branch from 5c3c9e1 to 64cbd9b Compare July 27, 2024 11:06
@mkm17 mkm17 marked this pull request as draft July 28, 2024 14:06
@mkm17 mkm17 force-pushed the issues/6103_spp_model_list branch from 64cbd9b to baeced2 Compare July 28, 2024 16:13
@mkm17 mkm17 marked this pull request as ready for review July 28, 2024 16:22
@milanholemans milanholemans self-assigned this Sep 6, 2024
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Nice start @mkm17, let's change a few things before we continue.

src/m365/spp/commands/model/model-list.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-list.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-list.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-list.spec.ts Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-list.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-list.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-list.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-list.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-list.mdx Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft September 7, 2024 19:12
@mkm17 mkm17 marked this pull request as ready for review September 14, 2024 08:10
Copy link
Contributor

@milanholemans milanholemans 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 noticed a few more things we should change @mkm17, after this it should be good to go.

src/m365/spp/commands/model/model-list.ts Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-list.mdx Outdated Show resolved Hide resolved
src/utils/spp.ts Outdated Show resolved Hide resolved
src/utils/spp.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-list.spec.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-list.spec.ts Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-list.mdx Show resolved Hide resolved
docs/docs/cmd/spp/model/model-list.mdx Show resolved Hide resolved
docs/docs/cmd/spp/model/model-list.mdx Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft September 19, 2024 20:44
@mkm17 mkm17 marked this pull request as ready for review September 26, 2024 15:34
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Almost there, let's do some final edits before we merge it.

src/utils/spp.spec.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-list.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-list.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-list.spec.ts Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-list.mdx Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft October 4, 2024 21:58
@mkm17 mkm17 force-pushed the issues/6103_spp_model_list branch from 1630898 to 17e650d Compare October 5, 2024 10:59
@mkm17 mkm17 marked this pull request as ready for review October 5, 2024 11:08
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

2 last remarks that I can fix while merging. Apart from this, it looks ready to be shipped!

});

await spp.assertSiteIsContentCenter(siteUrl);
await assert(stubGet.calledOnce);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await assert(stubGet.calledOnce);
assert(stubGet.calledOnce);

/**
* Asserts whether the specified site is a content center
* @param siteUrl The URL of the site to check
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add @throws error when site is not a content center.

@mkm17
Copy link
Contributor Author

mkm17 commented Oct 11, 2024

@milanholemans ok, thank you for all help. I will add also these suggestions to other spp commands

@milanholemans
Copy link
Contributor

Merged manually, thanks for the new command!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New command: m365 spp model list
2 participants