Replacing Classes with Mixins #6

Open
semmons99 opened this Issue Jan 31, 2011 · 3 comments

2 participants

@semmons99

During my lastest couse for RMU, I did some refactoring to Ruport to replace the Controller and Formatter Classes into Modules so they can be used as Mixins. I also extracted a lot of subclasses/submodules into their own space. Please check out https://github.com/semmons99/ruport and let me know if you're interested at merging these changes. If you are, I can work on updating the documentation that would be affected by these changes.

@Odaeus
Collaborator

Hi semmons99,

Thank you for working on improving Ruport. I've had a look through most of your commits and there are some I would be happy to pull in. Mostly relating to the separation of classes/modules into files.

There are a couple of issues that mean I'm hesitant to take all your commits:

  • I think the great re-indentation exercise of simmons99/ruport@0c7e445 would be quite disruptive to the project history and outweighs the limited benefit of replacing colons with separate module lines.
  • I'm also not sure about the advantages of making the very backwards-incompatible change of converting the Controller classes to modules?

Best regards,
Andrew

@semmons99

I think the great re-indentation exercise of semmons99/ruport@0c7e445 would be quite disruptive to the project history and outweighs the limited benefit of replacing colons with separate module lines.

You're probably right. Removing the colons helps with potential bugs when stuff isn't required in the correct order, but everything is working now, so it might not be worth it.

I'm also not sure about the advantages of making the very backwards-incompatible change of converting the Controller classes to modules?

This was done to provide future extensibility. Essentially, since you can only have one subclass, you'll need controller to get bigger and bigger if you need to add more methods, or have users subclass more specialized controllers. Conversely, you could just have multiple modules that they could pick and choose to include. I don't know the direction of Ruport, so I can't really say which approach is better.

@Odaeus
Collaborator

Your arguments seem quite reasonable, I suppose it would be a beneficial change in the long term. I would have to be careful how it's introduced due as it's such a big API change. Thanks, I'll have a think and might put a request for feedback on the Ruport mailing list tomorrow (you are welcome to this if you'd like!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment