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

Add title to filter schemas #4355

Conversation

@dougal83
Copy link
Contributor

dougal83 commented Jan 1, 2020

Adding titles to json filter schemas as groundwork toward openapi spec ref consolidation based on title property. See #4290

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
Copy link
Member

bajtos left a comment

Thank you @dougal83, I love this idea!

Let's discuss implementation details now.

@bajtos bajtos self-assigned this Jan 7, 2020
@bajtos bajtos changed the title Feat(repository json schema)add title to filter schemas Add title to filter schemas Jan 7, 2020
@bajtos bajtos mentioned this pull request Jan 7, 2020
3 of 4 tasks complete
@dougal83 dougal83 force-pushed the dougal83:feat(repository-json-schema)add-title-to-filter-schemas branch 2 times, most recently from 78604a9 to 8bfe446 Jan 8, 2020
@dougal83

This comment has been minimized.

Copy link
Contributor Author

dougal83 commented Jan 8, 2020

I've done a git rebase -i to fix history but that is probably a bad idea... not having a good evening.

@dougal83 dougal83 requested a review from bajtos Jan 9, 2020
Copy link
Member

bajtos left a comment

Lovely, the pull request is shaping up nicely 👍

Let's do few more iterations to iron out rough edges.

packages/openapi-v3/src/filter-schema.ts Outdated Show resolved Hide resolved
packages/openapi-v3/src/filter-schema.ts Outdated Show resolved Hide resolved
@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Jan 10, 2020

I've done a git rebase -i to fix history but that is probably a bad idea... not having a good evening.

No worries.

When working on a pull request, I usually run git rebase master to bring changes from the master branch, and then commit additional changes in new commits. This allows reviewers to look at incremental changes only.

Fortunately, your pull request is reasonably small, so it's not that difficult to review it in whole.

@dougal83 dougal83 requested a review from bajtos Jan 11, 2020
@bajtos
bajtos approved these changes Jan 21, 2020
Copy link
Member

bajtos left a comment

I quickly skimmed through the changes and don't see any obvious problems.

Let's get one more approval before landing please, I'll ping @strongloop/sq-lb-apex via Slack.

@dougal83 dougal83 force-pushed the dougal83:feat(repository-json-schema)add-title-to-filter-schemas branch from 05945db to 7a7cbd2 Jan 21, 2020
@dougal83

This comment has been minimized.

Copy link
Contributor Author

dougal83 commented Jan 21, 2020

@bajtos Thanks for the feedback. How do I resolve the commit lint failure?

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Jan 23, 2020

@dougal83

How do I resolve the commit lint failure

Huh, it looks like the linter does not understand that your pull request is adding a single commit only and as a result, it's linting existing commits on master. I vaguely remember that we have encountered this problem once or twice in the past, but that was years ago and I don't remember much details.

Let's try to rebase the pull request on top of the latest master to see if it helps.

add title property to filter schemas(filter, where, scope) in preparation for openapi schema consolidation

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
@bajtos bajtos force-pushed the dougal83:feat(repository-json-schema)add-title-to-filter-schemas branch from 7a7cbd2 to 1623c83 Jan 23, 2020
@bajtos bajtos merged commit 6105883 into strongloop:master Jan 23, 2020
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.001%) to 92.381%
Details
@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Jan 23, 2020

Rebase fixed the problem.

The pull request is landed, thank you @dougal83 for the contribution! ❤️

@dougal83

This comment has been minimized.

Copy link
Contributor Author

dougal83 commented Jan 23, 2020

@bajtos Thank you for sorting it out for me!

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.