-
Notifications
You must be signed in to change notification settings - Fork 1
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
Move responsibilities to DataContainer
class
#278
Comments
I agree with this, and I think this should be implemented. I also think that the first fields should be identical for each container, and then the specific arguments per container should follow after that. Also, I think that nomenclature should be aligned more. For example, Regarding breaking existing code, this is an argument that you have made a few times now (e.g. in issue #262). I don't think we have a large enough user base at this point to make that a prohibitive reason not to change things. We are very much still in the developmental phase of this project, and it's more important to "get things right", even if that means breaking changes, than ensuring backwards compatibility. I think these kinds of changes can be implemented in a separate PR though, so that this one can stay light, as you suggest. NOTE: comment copied from here |
Also, consider moving the methods from the current mixin-classes directly to |
This would bunch a whole load of unrelated methods together, making it very difficult to understand the separate functionalities and responsibilities. I would vote against that, unless we can reduce the total codebase to a few hundred lines. Also, I'd rather spend time working on new functionality rather than refactoring working stuff. |
See comments/discussion in #275
The text was updated successfully, but these errors were encountered: