Skip to content

Conversation

@Fivell
Copy link
Contributor

@Fivell Fivell commented Jan 17, 2017

this refactoring allows developers to add support of new schema types

Copy link
Collaborator

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Thanks for this. It makes a lot of sense to do it this way. The only change is to improve maintainability in the future and simplicity for the end-user.

README.md Outdated
end
end

JsonApiClient::Schema::TypeFactory.register money: MyMoneyCaster
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small change: having the users register on the TypeFactory class locks to this implementation. It'll most likely remain this way, but it might make more sense for the developer to call .register on the Schema class/module instead. If for whatever reason, we want to rename the TypeFactory class or refactor it, we need to keep that constant around. Internally, JsonApiClient::Schema.register could just delegate to JsonApiclient::Schema::TypeFactory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chingor13 , repushed.

this refactoring allows developers to add support of new schema types
@Fivell Fivell force-pushed the type_casting_refactoring branch from d535d55 to 724875d Compare January 17, 2017 23:59
@Fivell
Copy link
Contributor Author

Fivell commented Jan 18, 2017

@chingor13 , done!

@chingor13 chingor13 merged commit d301e30 into JsonApiClient:master Jan 18, 2017
@Fivell Fivell deleted the type_casting_refactoring branch January 18, 2017 07:30
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.

2 participants