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

feat: generating path examples #671

Merged
merged 31 commits into from
Oct 8, 2019
Merged

feat: generating path examples #671

merged 31 commits into from
Oct 8, 2019

Conversation

karol-maciaszek
Copy link
Contributor

@karol-maciaszek karol-maciaszek commented Oct 3, 2019

Closes #505

Checklist

  • Tests have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?

Feature

What is the current behavior? What is the new behavior?

Prism CLI lists available operation paths to the console. This feature adds example parameter values to those paths. This both path and query parameters.

Does this PR introduce a breaking change?

No

@karol-maciaszek karol-maciaszek marked this pull request as ready for review October 4, 2019 14:56
Copy link
Contributor

@XVincentX XVincentX 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 on the good track. Good job with the tests coverage.

  1. I've left some comments on the fp-ts usage and there are different ways we can improve the code. Let me know if you need any help on this

  2. Apparently the harness tests are halting (they go to timeout); do not worry about this too much right now but we'll have to figure it out :)

  3. I've tried the CLI with the new changes and here's the output:

image

I believe we have some changes to do:

http://127.0.0.1:4010/pet/findByStatus?status=available,sold,sold

This is technically correct but I believe that we should limit ourselves to a single example and more importantly do not repeat an example twice.

http://127.0.0.1:4010/pet/-41647052

This is also correct, although we might want to limit the example to positive numbers and less than…1000?

http://127.0.0.1:4010/user/quis%20velit%20nostrud

In order to make the things easier to read I would say we shall only produce examples with no spaces or special characters.

//cc @philsturgeon let me know if you're ok with these suggestions.

packages/cli/src/util/createServer.ts Show resolved Hide resolved
logInstance.note(`${resource.method.toUpperCase().padEnd(10)} ${address}${resource.path}`);
const path = pipe(
createExamplePath(resource),
Either.getOrElse(() => resource.path),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice fallback. If we fail, we display the regular path.

packages/cli/src/util/paths.ts Outdated Show resolved Hide resolved
packages/cli/src/util/paths.ts Outdated Show resolved Hide resolved
packages/http/src/mocker/serializer/style/delimited.ts Outdated Show resolved Hide resolved
packages/cli/src/util/__tests__/paths.spec.ts Outdated Show resolved Hide resolved
packages/cli/src/util/__tests__/paths.spec.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@XVincentX XVincentX left a comment

Choose a reason for hiding this comment

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

I had another round of checks; very good job, the code is going towards the right direction.

I've added other comments and suggestions and I'll take a final review tomorrow, and hopefully get this merged.

P.S: You have a lot of unused imports, try to check out all the files and remove them if you can.

CHANGELOG.md Outdated Show resolved Hide resolved
packages/cli/src/util/paths.ts Outdated Show resolved Hide resolved
packages/cli/src/util/paths.ts Outdated Show resolved Hide resolved
packages/cli/src/util/paths.ts Outdated Show resolved Hide resolved
packages/cli/src/util/paths.ts Outdated Show resolved Hide resolved
packages/cli/src/util/paths.ts Outdated Show resolved Hide resolved
packages/cli/src/util/paths.ts Show resolved Hide resolved
packages/http/src/mocker/generator/HttpParamGenerator.ts Outdated Show resolved Hide resolved
packages/http/src/mocker/generator/HttpParamGenerator.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@XVincentX XVincentX left a comment

Choose a reason for hiding this comment

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

Another shot of comments; these are mostly optional and we can take care of these separately.

Will merge this as soon as we have faker bug resolved.

packages/cli/src/util/paths.ts Outdated Show resolved Hide resolved
case HttpParamStyles.SpaceDelimited:
if (Array.isArray(value)) {
return Either.right(
serializeWithSpaceDelimitedStyle(spec.name, value as Array<string | number | boolean>, spec.explode),
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Suggested change
serializeWithSpaceDelimitedStyle(spec.name, value as Array<string | number | boolean>, spec.explode),
serializeWithSpaceDelimitedStyle(spec.name, value, spec.explode),

packages/cli/src/util/paths.ts Outdated Show resolved Hide resolved
packages/cli/src/util/paths.ts Outdated Show resolved Hide resolved
packages/cli/src/util/paths.ts Outdated Show resolved Hide resolved
packages/cli/src/util/paths.ts Show resolved Hide resolved
}
}

if (newSchema.type === 'string') {
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
if (newSchema.type === 'string') {
else if (newSchema.type === 'string') {

}
}

if (newSchema.type === 'object') {
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
if (newSchema.type === 'object') {
else if (newSchema.type === 'object') {

}
}

if (newSchema.type === 'array') {
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
if (newSchema.type === 'array') {
else if (newSchema.type === 'array') {

@XVincentX XVincentX merged commit de96a4f into master Oct 8, 2019
@XVincentX XVincentX deleted the feat/example-paths branch October 8, 2019 13:08
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.

Use path parameter example/default when displaying list of operations
2 participants