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

Deprecate .calculators #2216

Merged

Conversation

cbrunsdon
Copy link
Contributor

There are almost no reasonable reasons why these should be called
through the class models the way they are, near as I can tell they're
only ever used for display/form purposes in the backend.

@dhonig
Copy link

dhonig commented Sep 11, 2017

This makes sense. Going through the Config opposed to the class further separates the usage from the implementation. +1

@cbrunsdon cbrunsdon force-pushed the deprecate_calculator_dot_calculators branch 4 times, most recently from 3aac9c7 to 8edd3df Compare September 11, 2017 15:31
There are almost no reasonable reasons why these should be called
through the class models the way they are, near as I can tell they're
only ever used for display/form purposes in the backend.
@cbrunsdon cbrunsdon force-pushed the deprecate_calculator_dot_calculators branch from 8edd3df to 8dfedd2 Compare September 11, 2017 16:23
Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

Yeah, one level of indirection less!

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

It works for me! 👍

@gmacdougall gmacdougall merged commit 06874ef into solidusio:master Sep 13, 2017
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.

5 participants