Skip to content

feat(repository-json-schema): add filter title#4513

Merged
raymondfeng merged 2 commits into
loopbackio:masterfrom
dougal83:add-filter-items-title
Jan 27, 2020
Merged

feat(repository-json-schema): add filter title#4513
raymondfeng merged 2 commits into
loopbackio:masterfrom
dougal83:add-filter-items-title

Conversation

@dougal83

Copy link
Copy Markdown
Contributor

add title property to IncludeFilter items

Signed-off-by: Douglas McConnachie dougal83+git@gmail.com

See also #4355

Checklist

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

add title property to IncludeFilter items

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
Comment thread packages/repository-json-schema/src/__tests__/unit/filter-json-schema.unit.ts Outdated
Comment thread packages/repository-json-schema/src/__tests__/unit/filter-json-schema.unit.ts Outdated
title:
options.setTitle !== false
? `${modelCtor.modelName}.IncludeFilter.Items`
: undefined,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be ideal to not even add an undefined title property if setTitle === false.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

How about below? Code is harder to read but more concise.

@bajtos Would this be OK?

Amended:

  {
    ...(options.setTitle !== false && {
      title: `${modelCtor.modelName}.Filter`,
    }),
  }

From:

  {
    title:
      options.setTitle !== false ? `${modelCtor.modelName}.Filter` : undefined,
  }

do not set title unnecessarily, tests updated, added edge case.

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
@raymondfeng
raymondfeng merged commit c982164 into loopbackio:master Jan 27, 2020
@dougal83
dougal83 deleted the add-filter-items-title branch January 29, 2020 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants