Skip to content

feat(context): add @bind to decorate classes with binding attributes #2129

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
Merged
raymondfeng merged 2 commits into from
Dec 13, 2018
Merged

feat(context): add @bind to decorate classes with binding attributes #2129

raymondfeng merged 2 commits into from
Dec 13, 2018

Conversation

@raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Dec 6, 2018 •
edited
Loading

This PR is a reactivation of #992.

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@raymondfeng raymondfeng requested a review from bajtos as a code owner December 6, 2018 22:01
@raymondfeng raymondfeng mentioned this pull request Dec 6, 2018
Merged
7 tasks
@raymondfeng raymondfeng changed the title feat(context): add @bind to decorate classes with more information feat(context): add @bind to decorate classes with binding attributes Dec 6, 2018
@raymondfeng raymondfeng force-pushed the add-injectable-decorator branch 3 times, most recently from cf4e5a1 to bda048b Compare December 7, 2018 03:36
@bajtos
Copy link
Member

bajtos commented Dec 10, 2018

I read through the documentation, the proposal looks good at high level.

I'll try to find time to review it in more details later this week.

In the meantime, I'd like people from @strongloop/loopback-maintainers and especially @strongloop/sq-lb-apex to review these changes too.

@bajtos bajtos requested review from a team December 10, 2018 14:27
@raymondfeng raymondfeng force-pushed the add-injectable-decorator branch from bda048b to 891ea0d Compare December 10, 2018 23:03
bajtos
bajtos suggested changes Dec 11, 2018
View reviewed changes
@raymondfeng raymondfeng force-pushed the add-injectable-decorator branch 5 times, most recently from 2a44d96 to 8d9ebcd Compare December 12, 2018 22:11
@raymondfeng
Copy link
Contributor Author

raymondfeng commented Dec 12, 2018

@bajtos PTAL

bajtos
bajtos approved these changes Dec 13, 2018
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

LGTM.

Please get at least one more approval from @strongloop/loopback-maintainers before landing.

b-admike
b-admike reviewed Dec 13, 2018
View reviewed changes
Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

Some comments for tests in general (I'll review impl in another round). LGTM overall.

packages/context/test/acceptance/binding-decorator.feature.md
# Feature: @bind for classes representing various artifacts

- In order to automatically bind classes for various artifacts to a context
- As a developer
Copy link
Contributor

@b-admike b-admike Dec 13, 2018

Choose a reason for hiding this comment

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

Did we want the bullet points here? Or do we want to say something like As a developer, I want to decorate my classes to provide more metadata so that the bootstrapper can bind them to a context according to the metadata in order to automatically bind classes for various artifacts to a context.

Copy link
Contributor Author

@raymondfeng raymondfeng Dec 13, 2018

Choose a reason for hiding this comment

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

I follow other acceptance md files.

@raymondfeng raymondfeng force-pushed the add-injectable-decorator branch from 8d9ebcd to c93ce0a Compare December 13, 2018 15:50
@raymondfeng
Copy link
Contributor Author

raymondfeng commented Dec 13, 2018

@b-admike Thank you for the review comments. I have addressed most of them.

@raymondfeng raymondfeng merged commit 84c056e into master Dec 13, 2018
@raymondfeng raymondfeng deleted the add-injectable-decorator branch December 13, 2018 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@b-admike b-admike b-admike left review comments

@bajtos bajtos bajtos approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@raymondfeng @bajtos @b-admike