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 command spp model remove. Closes #6118 #6410

Closed
wants to merge 1 commit into from

Conversation

mkm17
Copy link
Contributor

@mkm17 mkm17 commented Oct 6, 2024

Adds command spp model remove. Closes #6118

@milanholemans
Copy link
Contributor

Thank you, well try to review it ASAP!

@mkm17 mkm17 force-pushed the issues/6118_spp_model_remove branch from 5afaf7c to 9a84d0e Compare October 13, 2024 20:55
@milanholemans milanholemans self-assigned this Oct 17, 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 stuff, I made a few comments along the way. Could you take a look at them, please?

src/m365/spp/commands/model/model-remove.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.ts Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-remove.mdx Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft October 17, 2024 20:57
@mkm17 mkm17 force-pushed the issues/6118_spp_model_remove branch from 9a84d0e to 53f4c24 Compare October 24, 2024 19:16
@mkm17 mkm17 marked this pull request as ready for review October 24, 2024 19:21
@Adam-it Adam-it added the hacktoberfest-accepted Accept for hacktoberfest, will merge later label Oct 29, 2024
@Adam-it
Copy link
Member

Adam-it commented Oct 29, 2024

@mkm17 I added the hacktoberfest-accepted label to this PR which means that this PR will count as done for the Hacktoberfest event. So if you participate in this event it will get you unblocked and it allows us to merge this PR later when we catch up 👍
Thanks for your support and awesome contribution 👏 You Rock 🤩

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 work, let's do a couple of more changes.

src/m365/spp/commands/model/model-remove.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.ts Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.spec.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.spec.ts Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-remove.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-remove.mdx Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft November 10, 2024 21:59
@mkm17 mkm17 force-pushed the issues/6118_spp_model_remove branch from 53f4c24 to e4e531d Compare November 17, 2024 16:28
@mkm17 mkm17 marked this pull request as ready for review November 17, 2024 20:56
@mkm17
Copy link
Contributor Author

mkm17 commented Nov 17, 2024

Hi @milanholemans, ok thank you for the review. You are right, I now understand the odata.null case.

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.

We're almost there, let's do a couple of more changes.

docs/docs/cmd/spp/model/model-remove.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-remove.mdx Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.ts Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-remove.ts Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft November 19, 2024 23:05
@mkm17 mkm17 force-pushed the issues/6118_spp_model_remove branch from e4e531d to 3e2c005 Compare November 21, 2024 22:03
@mkm17
Copy link
Contributor Author

mkm17 commented Nov 21, 2024

@milanholemans I hope the changes to the tests are fine. I've included the same check for the .classifier suffix as in the model get command.

@mkm17 mkm17 marked this pull request as ready for review November 21, 2024 22:07
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.

Apart from a few tiny remarks, it looks good to me!

option: '-t, --title [title]'
},
{
option: '-f --force'
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
option: '-f --force'
option: '-f, --force'

public async commandAction(logger: Logger, args: CommandArgs): Promise<void> {
try {
if (!args.options.force) {
const confirmationResult = await cli.promptForConfirmation({ message: `Are you sure you want to remove model '${args.options.title ? args.options.title : args.options.id}'?` });
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
const confirmationResult = await cli.promptForConfirmation({ message: `Are you sure you want to remove model '${args.options.title ? args.options.title : args.options.id}'?` });
const confirmationResult = await cli.promptForConfirmation({ message: `Are you sure you want to remove model '${args.options.title ?? args.options.id}'?` });

assert(stubDelete.calledOnce);
});

it('deletes model by title with .classifier suffic', async () => {
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
it('deletes model by title with .classifier suffic', async () => {
it('deletes model by title with .classifier suffix', async () => {

it('deletes model by title with .classifier suffic', async () => {
const stubDelete = sinon.stub(request, 'delete').callsFake(async (opts) => {
if (opts.url === `https://contoso.sharepoint.com/sites/portal/_api/machinelearning/models/getbytitle('modelname.classifier')`) {
return '';
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
return '';
return;

it('deletes model by title', async () => {
const stubDelete = sinon.stub(request, 'delete').callsFake(async (opts) => {
if (opts.url === `https://contoso.sharepoint.com/sites/portal/_api/machinelearning/models/getbytitle('modelname.classifier')`) {
return '';
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
return '';
return;

@milanholemans
Copy link
Contributor

Merged manually, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accept for hacktoberfest, will merge later pr-merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New command: m365 spp model remove
3 participants