#99 feat: Include argument 'comment' in assertions

[PetterHopp]

I suggest to include the argument 'comment' in assertion functions.
This suggestion appends the comment after the standard message and may be considered as a solution of
issue #99. The original wish was to include the comment before the standard message.
The changes are partly tested, but I have not been able to perform a full test of all changes (all assertion functions will be changed).
Be aware that I suggest to put the argument 'comment' before the argument 'add'. the reason is that I think this is a logical order. However, this my introduce breaking changes if users have been giving the arguments to assertion functions without using the name of the argument, i.e. not written "add = collection" but only "collection".

[PetterHopp]

I had a look at the failed test for Windows. The test failed due to too many arguments for a function which I believe is OK as I have added an argument.

[mllg]

Thanks for the PR. I'll first try to release a new version of checkmate in 2-3 weeks, then review and merge this one.

[PetterHopp]

Hi I'm glad that you will try to make a new release soon, I'm very interested in some of the new features in 2.1. Thank you for considering the PR and no problems with prioritizing the new release first. Kind regards, Petter Hopp ***@***.*** Fra: Michel Lang ***@***.***> Sendt: fredag 24. september 2021 10:52 Til: mllg/checkmate ***@***.***> Kopi: Hopp, Petter ***@***.***>; Author ***@***.***> Emne: Re: [mllg/checkmate] #99 feat: Include argument 'comment' in assertions (#211) Thanks for the PR. I'll first try to release a new version of checkmate in 2-3 weeks, then review and merge this one. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#211 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AC7JXNTM5DZG6B6RLHQ2B6TUDQ3ZHANCNFSM5C3CVEGQ>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[novica]

Hi. Is this PR going to be merged?

[renozao]

@PetterHopp I would also love to see this capability too!
For checks that happens in functions called by the end-user, I think it is must to provide a more specific and informative error message that relates to the nature/purpose of the arguments and function.

For a while in my work, I define dynamic wrappers (assert_*_msg) around all existing checkmate functions to get this capability and it proved to provide great value, both for the developer and the end-user.

@mllg would you consider merging the PR?

Some comments/suggestions:

• This is a capability that is provided by argument msg in assertthat::assert_that() or info in the expect_* functions. One might consider using one of these 2 established names instead of comment.
• One could have another argument append = TRUE|FALSE, which would control whether the custom message is appended to the standard message or should replace it. This enables building cleaner messages in cases where the standard error message gets very busy listing lots of values.

Thanks! 🙏

[PetterHopp]

@renozao Thank you for supporting the possibility of adding a comment in assertion functions.
I agree that comment may not be the best name for the argument. Of the suggestions msg and info, I would prefer msg. info is used expect-functions as in package testthat, but the argument is soft-deprecated in testthat and one is advised to not use it in new code.
The argument append = TRUE|FALSE is an excellent idea that I fully support.

[tdeenes]

@PetterHopp @mllg I would rather use fmt as the name of the argument, and instead of introducing an other argument append, I suggest that fmt shall be the same as the fmt argument of base::sprintf(). Then, makeAssertion would look like this:

makeAssertion <-  function (x, res, var.name, collection, fmt = NULL) {
    if (!isTRUE(res)) {
        assertString(var.name, .var.name = ".var.name")
        assertString(fmt, .var.name = "fmt", null.ok = TRUE)
        if (is.null(collection)) {
            if (is.null(fmt)) {
              fmt <- "Assertion on '%s' failed: %s." 
            }
            mstop(fmt, var.name, res, call. = sys.call(-2L))
        }
        assertClass(collection, "AssertCollection", .var.name = "add")
        if (is.null(fmt)) {
          fmt <- "Variable '%s': %s."
        }
        collection$push(sprintf(fmt, var.name, res))
    }
    return(invisible(x))
}

One could then customize how the error message is structured, e.g.:

object_to_test <- "a"
custom_fmt <- "Argument '%1$s' is incorrect and must be fixed. Details: %2$s"
assert_int(object_to_test, fmt = custom_fmt)