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

refactor(Container): move defineRelations method to Mapper #345

Merged
merged 1 commit into from Jun 2, 2016
Merged

refactor(Container): move defineRelations method to Mapper #345

merged 1 commit into from Jun 2, 2016

Conversation

stalniy
Copy link
Contributor

@stalniy stalniy commented Jun 2, 2016

No description provided.

@codecov-io
Copy link

codecov-io commented Jun 2, 2016

Current coverage is 93.30%

Merging #345 into master will decrease coverage by <.01%

@@             master       #345   diff @@
==========================================
  Files            18         19     +1   
  Lines          2252       2254     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2102       2103     +1   
- Misses          150        151     +1   
  Partials          0          0          

Powered by Codecov. Last updated by ef19cc5...7b91154

@jmdobry
Copy link
Member

jmdobry commented Jun 2, 2016

Looks good, though after looking through the changes, I'm not sure I fully understand the reasoning behind them. Can you comment here (or update the description of the PR) with a description of the purpose of these changes?

@stalniy
Copy link
Contributor Author

stalniy commented Jun 2, 2016

@jmdobry
Notes:

  • relations.js was added as a single entry point for all relations. So, then you safely can import Relation class and be sure that all relation types are included as well. Don't want to keep this logic inside decorators.js as this has nothing to do with decorators
  • it's useful to have static methods on Relation class as we can hide internal implementation of all relation types
  • I want to hide all relation logic inside Relation class
  • defineRelations was moved from Container to Mapper because relation definitions is something what relates more to Mapper than to Container
  • also it's better to replace if-s logic with polymorphism. As a result in future, when you add a new relation type, you won't need to change code inside Mapper.js.

@jmdobry
Copy link
Member

jmdobry commented Jun 2, 2016

perfect

@jmdobry jmdobry merged commit 6d79c4f into js-data:master Jun 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants