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 spp model get command Closes #6105 #6409

Closed
wants to merge 1 commit into from

Conversation

mkm17
Copy link
Contributor

@mkm17 mkm17 commented Oct 6, 2024

Adds spp model get command Closes #6105

@milanholemans
Copy link
Contributor

Thank you, well try to review it ASAP!

@mkm17 mkm17 force-pushed the issues/6105_m365_spp_model_get branch from 4e27dc2 to 13ca6fd Compare October 13, 2024 20:56
@mkm17 mkm17 marked this pull request as draft October 25, 2024 07:19
@mkm17
Copy link
Contributor Author

mkm17 commented Oct 25, 2024

I have converted it to draft, as there are some conflicts to be resolved.

@mkm17 mkm17 force-pushed the issues/6105_m365_spp_model_get branch from 13ca6fd to 83dcfdd Compare October 25, 2024 20:26
@mkm17 mkm17 marked this pull request as ready for review October 25, 2024 20:38
@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 🤩

@milanholemans milanholemans self-assigned this Nov 16, 2024
@mkm17 mkm17 force-pushed the issues/6105_m365_spp_model_get branch from 83dcfdd to 3483a1e Compare November 17, 2024 21:09
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 proceed.

src/m365/spp/commands/model/model-get.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-get.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-get.ts Show resolved Hide resolved
src/m365/spp/commands/model/model-get.ts Outdated Show resolved Hide resolved
src/m365/spp/commands/model/model-get.ts Show resolved Hide resolved
docs/docs/cmd/spp/model/model-get.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-get.mdx Show resolved Hide resolved
docs/docs/cmd/spp/model/model-get.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-get.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-get.mdx Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft November 18, 2024 14:26
@mkm17 mkm17 force-pushed the issues/6105_m365_spp_model_get branch from 3483a1e to 7d3db71 Compare November 21, 2024 20:58
@mkm17
Copy link
Contributor Author

mkm17 commented Nov 21, 2024

@milanholemans thank you again for the review. I believe I have addressed all the suggested changes. I have one question regarding the tests: I updated it to assert.strictEqual(JSON.stringify(loggerLogSpy.lastCall.args[0]), JSON.stringify(modelResult)); instead of the previous approach. Is that ok?

@mkm17 mkm17 marked this pull request as ready for review November 21, 2024 21:02
@milanholemans
Copy link
Contributor

@milanholemans thank you again for the review. I believe I have addressed all the suggested changes. I have one question regarding the tests: I updated it to assert.strictEqual(JSON.stringify(loggerLogSpy.lastCall.args[0]), JSON.stringify(modelResult)); instead of the previous approach. Is that ok?

Is there a reason why we stringify everything instead of using assert.deepStrictEqual?

@mkm17 mkm17 force-pushed the issues/6105_m365_spp_model_get branch from 7d3db71 to 8caeee1 Compare November 22, 2024 07:49
@mkm17
Copy link
Contributor Author

mkm17 commented Nov 22, 2024

@milanholemans I saw this method somewhere in the code, but definitely it is a much better idea. Thank you.

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.

It's looking good, we're almost there.

docs/docs/cmd/spp/model/model-get.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-get.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/spp/model/model-get.mdx Outdated Show resolved Hide resolved
src/utils/spp.ts Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft November 24, 2024 19:39
@mkm17 mkm17 force-pushed the issues/6105_m365_spp_model_get branch from 8caeee1 to bb89289 Compare December 1, 2024 21:16
@mkm17
Copy link
Contributor Author

mkm17 commented Dec 1, 2024

@milanholemans Thank you for the review. The changes have been applied to the new version.

@mkm17 mkm17 marked this pull request as ready for review December 1, 2024 21:23
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 one tiny enhancement, it looks good to me.

<Tabs>
<TabItem value="JSON">

```json
Copy link
Contributor

Choose a reason for hiding this comment

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

This response is looking gigantic right now. Let's try to trim it down a bit by:

  • For arrays, let's show only 1 object in the array.
  • Right now, we have multiple fields in the model like: address, discount percentage, date of change, ... Let's show only one of those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@milanholemans ok, it is done now, the response should be shorter :)

@mkm17 mkm17 force-pushed the issues/6105_m365_spp_model_get branch from bb89289 to 2d71fdb Compare December 17, 2024 21:37
@milanholemans
Copy link
Contributor

Merged manually, thanks!

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 get
3 participants