Method rule return true for failure and false for success, why's that?

[pr4bh4sh]

Hi, Currently, I'm creating a bunch of custom gherkin lint rule using cuke_linter.

1. I noticed that when a gherkin files content adheres to the expectations of the rule it the method rule return false and true in case of failure. It doesn't feel right/natural.

def rule(model)
    complies_with_the_rule = ... some logic to check the rule ...

    return complies_with_the_rule ? success : failure

    vs

    return complies_with_the_rule ? false : true
end

2. Method name rule doesn't feel right, run, execute, check will represent the intention better. Although, I may have formed this opinion because I evaluated many other gherkin linter that uses run or execute as well as my inclination towards verb for the method names. Feel free to ignore this question.

So far it has been a pleasure using this gem, thanks for creating it. I'm not creating this issue to say it's bad or to criticise you, that's not my intention.

I just had an odd feeling about it and just wanted to know if there was a specific reason behind it?

[pr4bh4sh]

Just additional info, this is how I have mitigated for now

class BaseLinter < CukeLinter::Linter
  def rule(model)
    result = check(model)
    case result
      when :good, :reject
        false
      when :bad
        true
      else
        raise("Must return :good, :bad or :reject, recieved: #{result.inspect}")
    end
  end

  def check(_)
    raise(NotImplementedError, 'Please implement the method')
  end
end

class SomeCustomLinter < BaseLinter
  def name
    'SomeCustomLinter'
  end

  def message
    'My custom message'
  end

  def check(model)
    # Let's assume that rule only applies for scenario
    return :reject unless model.is_a?(CukeModeler::Scenario)

    error_found = ...
    ...
    evaluate the model
    ... 

    error_found ? :bad : :good
  end
end

[enkessler]

In your head, your asked the question "Does this element comply with the rule?". In my head, I asked "Did the rule find a violation?". It could have just as easily been the other way around. Alternatively, think of it this way: when testing for problems, negative test results are a good thing. No one wants to get a phone call that starts off with "Hey, we found something...".

So, yeah, that's essentially it. I'm not inclined to change it from one to the other but your monkey patch certainly does the trick. My only call out there is that if you decide that any of your custom rules are worth contributing back to the gem then they will need to be refactored so that they follow the expected format. Returning an enumerated type instead of true/false could be handy in the future but, so far, it hasn't been important to know why a rule didn't find a problem (e.g because the model is not an applicable type) just that it didn't find any problem.

Adding some method aliases, on the other hand, is a perfectly acceptable idea. It would be backwards compatible and it might help people to more intuitively wrap their heads around the expected true/false pattern if they could use a method name that suggested that a positive response is bad or, at the very least, didn't suggest that a positive response is good.

[pr4bh4sh]

Thanks for the answer. I understand your point of view. I believe this thread will serve as a good reference point to clarify and provide an alternative way to align their expectation with a method return if someone in future has a similar question or different point of view.

I do plan to contribute back some of the rules, and I'll modify them to follow the current rule pattern of the gem.

I think my question is answered and this issue can be closed now.

[enkessler]

I'm always glad to help! :)