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

Enhances flow get #5913

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MathijsVerbeeck
Copy link
Contributor

Closes #4683

I did not update the doc file with the entire output of a command, as this can get very long. I ran the command with --output json on a very small flow with just four actions and it returned more than 15000 lines of details. This would make the doc file way too large I think.

@milanholemans
Copy link
Contributor

Thank you @MathijsVerbeeck, we'll try to review it soon.

@martinlingstuyl martinlingstuyl self-assigned this May 3, 2024
src/m365/flow/commands/flow-get.spec.ts Outdated Show resolved Hide resolved
docs/docs/cmd/flow/flow-get.mdx Outdated Show resolved Hide resolved
@MathijsVerbeeck
Copy link
Contributor Author

@martinlingstuyl I've changed your requests. Could you please have another look?

@martinlingstuyl
Copy link
Contributor

About that mock file @MathijsVerbeeck, that now seems to contain 14000+ lines. Is it an idea if we use the same json as used in the docs? To get down on the nr of lines?

@MathijsVerbeeck
Copy link
Contributor Author

About that mock file @MathijsVerbeeck, that now seems to contain 14000+ lines. Is it an idea if we use the same json as used in the docs? To get down on the nr of lines?

That wouldn't cover all 'branches' of code, as we add some things to the response at lines 103-104 in flow-get.ts, so then we wouldn't have all code coverage 😄

@martinlingstuyl
Copy link
Contributor

Ok, I see... maybe we could switch it to a manual trigger though? That would remove quite some code I believe, among other things the whole connectionReferences node.

@MathijsVerbeeck
Copy link
Contributor Author

We could, yes, but then I perhaps will have to add a part that tests this code manually to the mock. Is that fine?

@MathijsVerbeeck
Copy link
Contributor Author

@martinlingstuyl PR should be ready to review. I've changed the flow-get mock file.

Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Looks fine @MathijsVerbeeck. 👏 Much better like this also in terms of changed lines.

@@ -10,8 +10,10 @@ import { session } from '../../../utils/session.js';
import { sinonUtil } from '../../../utils/sinonUtil.js';
import commands from '../commands.js';
import command from './flow-get.js';

import { flowGetResponse } from './flow-get.mock.js';
describe(commands.GET, () => {
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 an extra white line above describe

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

Successfully merging this pull request may close these issues.

Enhancement: Include additional flow properties returned with flow get by adding "$expand" query parameter
3 participants