Enable logging non-string - fix #1410

[vincent-psarga]

Summary

See #1410

Details

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

• Refactor (code change that does not change external functionality)
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• I've added tests for my code
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

[olleolleolle]

Looks good to me!

[aslakhellesoy]

Why the dup?

[vincent-psarga]

Otherwise we log the object itself and it will display its value at display time, not at log time (there a test showing that and it fails without the dup)

[aslakhellesoy]

I understand why we used to do message.dup, but I don't think we need message.to_s.dup. I think message.to_s is sufficient.

[olleolleolle]

(Me, a reader, thought this was because the string would be used in a mutable context.)

[vincent-psarga]

I though the to_s would be enough too. In fact it's enough for non-string objects (obviously :D ) but when using a String we need the dup.

An example from the tests:

        describe 'when modifying the printed variable after the call to puts' do
          define_feature <<-FEATURE
        Feature: Banana party

          Scenario: Monkey eats banana
            When puts is called twice for the same variable
          FEATURE

          define_steps do
            When(/^puts is called twice for the same variable$/) do
              foo = String.new('a')
              puts foo
              foo.upcase!
              puts foo
            end
          end

This test fails without the dup (we output A\nA instead of a\nA). I suspect that String.to_s do not return a duplicate but imply itself (also note, the test still uses puts instead of log, but that'll be removed once we removed the override of puts by log)

And @olleolleolle you're not wrong, it's just that the string might be mutated between the moment it I logged and the moment we actually display it.

[aslakhellesoy]

Is Cucumber::LogTypeInvalid still used? Should it be removed?

[vincent-psarga]

indeed