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.

Sign up for GitHub

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

Jump to bottom

feat(repository): helper function for getting Model metadata #813

Merged
merged 1 commit into from
Jan 2, 2018
Merged

feat(repository): helper function for getting Model metadata #813

merged 1 commit into from
Jan 2, 2018

Conversation

kjdelisle
Copy link
Contributor

Description

A helper function to roll the properties metadata into one object with the model's metadata.

Related issues

connected to #786

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide

kjdelisle
kjdelisle commented Dec 20, 2017
View reviewed changes
packages/repository/test/unit/decorator/metadata.ts Outdated
);
});

it('retrieves metadata for classes with @model and property', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to change this to say @property.

@kjdelisle kjdelisle force-pushed the repository/inspect-all-class-metadata branch 2 times, most recently from feda037 to 0bf517a Compare December 20, 2017 16:17
raymondfeng
raymondfeng reviewed Dec 20, 2017
View reviewed changes
packages/repository/src/decorators/metadata.ts Outdated
import {MODEL_KEY, MODEL_PROPERTIES_KEY} from '../';
import {ModelDefinition, PropertyDefinition} from '../../index';

export namespace ModelMetadata {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to create a ModelMetadataHelper class and make getModelMetadata a static method.

Since there is only one method, we can just simply export the getModelMetadata function.

IIRC, the naming convention for namespace should be lower case. ModelMetadata.getModelMetadata() will make us assume that ModelMetadata is a class and getModelMetadata is a static method.

raymondfeng
raymondfeng reviewed Dec 20, 2017
View reviewed changes
packages/repository/src/decorators/metadata.ts Outdated
const meta = new ModelDefinition(
Object.assign(
{
name: '<unknown>', // Name is a required field.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we at least make the default value of name to be target.name?

@raymondfeng
Copy link
Contributor

@kjdelisle Please fix the commit message:

⧗   input: fixup: Use class instead of namespace

✖   type must be one of [build, chore, ci, docs, feat, fix, perf, refactor, revert, style, test] [type-enum]

✖   found 1 problems, 0 warnings

⧗   input: feat(repository): helper function for getting Model metadata

✔   found 0 problems, 0 warnings

@kjdelisle kjdelisle force-pushed the repository/inspect-all-class-metadata branch from 5aed1d4 to d1e6381 Compare December 20, 2017 16:54
@kjdelisle kjdelisle force-pushed the repository/inspect-all-class-metadata branch 2 times, most recently from 2c70c8b to c5d4114 Compare December 20, 2017 18:07
@kjdelisle
Copy link
Contributor Author

@slnode test please

raymondfeng
raymondfeng reviewed Dec 20, 2017
View reviewed changes
packages/repository/src/decorators/metadata.ts Outdated
* the output of the metadata retrieval functions.
*/
static getModelMetadata(target: Function, options?: InspectionOptions) {
const classDef = MetadataInspector.getClassMetadata(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to cache the result on the target? See an example at https://github.com/strongloop/loopback-next/blob/master/packages/rest/src/router/metadata.ts#L177

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added some code to alter the MODEL_KEY/target metadata if there aren't any properties defined on first call.

@kjdelisle kjdelisle force-pushed the repository/inspect-all-class-metadata branch from c5d4114 to 7e19fe1 Compare December 20, 2017 20:45
raymondfeng
raymondfeng reviewed Dec 20, 2017
View reviewed changes
packages/repository/src/decorators/metadata.ts Outdated
*/
static getModelMetadata(target: Function, options?: InspectionOptions) {
const classDef = MetadataInspector.getClassMetadata(
MODEL_KEY,
Copy link
Contributor

@raymondfeng raymondfeng Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should use a different key to cache the fully built model definition and leave MODEL_KEY to reference the raw spec from @model.

raymondfeng
raymondfeng reviewed Dec 20, 2017
View reviewed changes
packages/repository/src/decorators/metadata.ts Outdated
classDef,
),
);
if (_.isEmpty(_.keys(meta.properties))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a side effect of reusing MODEL_KEY for in-place calculation?

raymondfeng
raymondfeng requested changes Dec 20, 2017
View reviewed changes
Copy link
Contributor

@raymondfeng raymondfeng left a comment

Choose a reason for hiding this comment

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

Please see my latest comments.

@jannyHou jannyHou mentioned this pull request Jan 2, 2018
Merged
@kjdelisle kjdelisle force-pushed the repository/inspect-all-class-metadata branch 3 times, most recently from 0bd351a to e6cb743 Compare January 2, 2018 18:43
@raymondfeng
Copy link
Contributor

Please fix the commit log.

@kjdelisle kjdelisle force-pushed the repository/inspect-all-class-metadata branch from e6cb743 to 4a293d3 Compare January 2, 2018 19:07
@kjdelisle
Copy link
Contributor Author

@slnode test please

@shimks
Copy link
Contributor

shimks commented Jan 2, 2018

@slnode test please

jannyHou
jannyHou approved these changes Jan 2, 2018
View reviewed changes
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kjdelisle kjdelisle merged commit b19635d into master Jan 2, 2018
@kjdelisle kjdelisle deleted the repository/inspect-all-class-metadata branch January 2, 2018 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shimks shimks shimks approved these changes

@b-admike b-admike b-admike approved these changes

@jannyHou jannyHou jannyHou approved these changes

@raymondfeng raymondfeng raymondfeng approved these changes

@bajtos bajtos Awaiting requested review from bajtos

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kjdelisle @raymondfeng @shimks @jannyHou @b-admike