whitequark / ast

A library for working with Abstract Syntax Trees.
MIT License
195 stars 25 forks source link

Move Processor Behavior to a Module #5

Closed teliosdev closed 9 years ago

teliosdev commented 9 years ago

I don't know if this is an acceptable pull request because of the lack of a contributing file, but I wanted to modify the way the processor worked a little bit. My primary concern with the processor was that it was a class - you were forced to subclass the processor, which, in my mind, made no sense. The processor didn't make any sense as a class, mainly because of these reasons:

  1. It violates the Liskov substitution principle, because the processor should never be instantiated within a program (it is always subclassed).
  2. It's an anti-pattern. Specifically, it's the framework superclass anti-pattern.
  3. It violates the Composite resuse principle, or "composition over inheritance."

These are made complicated by the fact that the behaviors of the processor are essentially private. the #process and #process_all methods are not meant to be public, so describing how the processor "behaves" publicly is awkward.

However, in interest of compatibility, I've kept the processor as a class, and instead defined AST::Processor::Module, which contains the previous processor behavior. AST::Processor is now just a class that includes AST::Processor::Module.

Anyway, nice library. Cheers!

teliosdev commented 9 years ago

It looks like the build is failing for reasons independent to this change.

whitequark commented 9 years ago

Yes, you have a point. Two nits:

teliosdev commented 9 years ago

I agree with both points; how should I deprecate AST::Processor?

whitequark commented 9 years ago

# @deprecated message...

teliosdev commented 9 years ago

Done.

whitequark commented 9 years ago

Process is a really awful name. It's a verb, and a module is a thing.

teliosdev commented 9 years ago

I have to say, I'm extremely happy with the amount of documentation that you provide in your library.

whitequark commented 9 years ago

Renamed to Mixin instead and merged. Thanks!

teliosdev commented 9 years ago

What version can I expect to find this in?

whitequark commented 9 years ago

Released 2.1.0.

teliosdev commented 9 years ago

Awesome, thanks!