maetl / calyx

A Ruby library for generating text with recursive template grammars.
MIT License
61 stars 5 forks source link

Behaviors of "Modifiers" are different from documentation #21

Closed tra38 closed 7 years ago

tra38 commented 7 years ago
module Pluralize
  def pluralize_cats
    if self.to_i == 1
      "#{self} cat"
    else
      "#{self} cats"
    end
  end
end

class CatWatch < Calyx::Grammar
   modifier Pluralize
   start "Today, I saw {number.pluralize_cats} on the way to work."
end

reporter = CatWatch.new
reporter.generate(number: 1)
#=>ArgumentError: wrong number of arguments (given 1, expected 0)

To resolve this issue, I had to modify #pluralize_cats slightly...

module Pluralize
   def pluralize_cats(string)
      if string.to_i == 1
         "#{string} cat"
      else
         "#{string} cats"
      end
   end
end

class CatWatch < Calyx::Grammar
   modifier Pluralize
   start "Today, I saw {number.pluralize_cats} on the way to work."
end

reporter = CatWatch.new
reporter.generate(number: 1)
#=>Today, I saw 1 cat on the way to work.

Oddly, when I copy and paste the example Modifier Mixer code in IRB (which states that I should use self), I also get the ArgumentError: wrong number of arguments (given 1, expected 0).

I originally wrote this code in Ruby 2.4.0, but I tested it as far back as 2.2.2, and still got that same error.

maetl commented 7 years ago

Thanks. I’ll explore the problem now and look to fix this for version 0.15—might require more detail in the docs to clarify anything that’s potentially confusing.

maetl commented 7 years ago

Looks like there’s a mistake in the docs. My apologies for the confusion this caused.

There’s already a test which defines the correct behaviour that demonstrates how it’s supposed to work, which is the same as your second example above.

In terms of the original API decision, there are several things I wanted to achieve here:

  1. Avoid monkeypatching strings or string extensions at runtime. Monkeypatching will always be possible if that’s what authors want to do, but it’s good to have an alternative. This makes Calyx a bit safer for use in larger applications where it might be less desirable to cause application-wide changes to the core string object.
  2. Avoid internal class/instance state and use pure functions for string processing where possible. Pure functions have no side-effects and their output is entirely defined by their input arguments. They’re easier to test and easier to share across projects. I felt this would be the best way to embed other string libraries—eg: Inflecto.

For some reason it didn’t get documented properly (possibly due to copy/pasting example code blocks). I’ll make sure this gets fixed.

tra38 commented 7 years ago

Since c83376 fixes the documentation problem, I'm going to close this issue. Thanks for your help, @maetl!