raise an error on warnings in tests

[mockdeep]

No description provided.

[mockdeep]

This is an experiment to see what will happen.

[mockdeep]

@dlemstra I've been able to fix most of the warnings on this branch, but there are still a couple that I'm not sure about. Do you have any idea on the remaining ones?

[dlemstra]

Just pushed some extra changes to fix the errors. Now wondering if we should not make a setting for"treat warning as errors" in the code (instead of overriding stderr.write) so our users can also do this? The following 'hack' could make this possible:

void
rm_warning_handler(const ExceptionType severity, const char *reason, const char *description)
{
    if (getenv("RMAGICK_WARNING_IS_ERROR"))
    {
        rm_error_handler(severity, reason, description);
        return;
    }

    rb_warning("RMagick: %s%s%s",
        GetLocaleExceptionMessage(severity, reason),
        description ? ": " : "",
        description ? GetLocaleExceptionMessage(severity, description) : "");
}

Or we could introduce a new method that sets a variable that is used here. Your thoughts?

[mockdeep]

Nice! Thanks for looking into it. I wonder, would it be reasonable to just throw an error in this cases rather than allowing them to configure it? If they're using the library wrong, I guess my preference would be to fail fast and loud. Warnings have a way of slipping through the cracks. I suppose we could also have something like your suggestion as a transitional tool:

• 4.x -> add ability to toggle on
• 5.x -> turn it on by default
• 6.x -> remove the option

[dlemstra]

Maybe it would be better to allow a user to set warning handling to one of the following values: Quiet, Warnning or Error? I don't think it will be a good idea to force a user to treat a warning as error. Some places might raise a warning where the user does not care about.

[mockdeep]

Yeah, I guess it depends on the severity of it. In general, if they're using the library wrong I'd prefer an error. If it's just something that should work but might produce results that are a little odd, maybe a warning makes more sense. Even in those cases, though, I might still throw an error and push them to be more explicit about what they want. If they don't care, they can always rescue the error in their code. It might be more productive to discuss these on a case-by-case basis, though.

[dlemstra]

I am just a bit worried that this will break existing code of a user. Maybe we could still allow the 3 choices and set the default to raise an error?

[mockdeep]

Yeah, that makes sense. I think it would be important to have a transition plan. Rather than immediately starting to throw an error by default, we probably want to default it to warning like it does now, and allow a user to configure it. If we want to make throwing an error the default, we can add a deprecation warning in the 4.x version and in 5.0 we can make it the new default.