New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved exception messages (close #184) #677

Merged
merged 2 commits into from Apr 20, 2017

Conversation

Projects
None yet
5 participants
@coatless
Contributor

coatless commented Apr 19, 2017

This is PR 3 of 3.

The objective of this PR is to upgrade existing exception messages that were identified in #184 as being candidates for dynamic information.

(PR 1 (#674) included the upstream refresh of the tinyformat library. PR 2 (#676) put in place the exception infrastructure used in this build.)


Changes:

  • Upgraded exceptions as highlighted by the table in #184
  • Corrected exception throws from not_compatible() to not_a_matrix() in Matrix.h (one was missing a throw!)
  • Corrected doxygen documentation in S4 regarding error thrown.
@@ -29,7 +29,10 @@ namespace Rcpp {
namespace internal {
template <typename T> T primitive_as(SEXP x) {
if (::Rf_length(x) != 1) throw ::Rcpp::not_compatible("expecting a single value");
if (::Rf_length(x) != 1) {

This comment has been minimized.

@coatless

coatless Apr 19, 2017

Contributor

Should this be changed to Rf_xlength()?

@coatless

coatless Apr 19, 2017

Contributor

Should this be changed to Rf_xlength()?

This comment has been minimized.

@kevinushey

kevinushey Apr 19, 2017

Contributor

Good catch -- I believe it should be.

@kevinushey

kevinushey Apr 19, 2017

Contributor

Good catch -- I believe it should be.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Apr 19, 2017

Codecov Report

Merging #677 into master will decrease coverage by 3.05%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #677      +/-   ##
==========================================
- Coverage   92.88%   89.82%   -3.06%     
==========================================
  Files          65       66       +1     
  Lines        3303     3509     +206     
==========================================
+ Hits         3068     3152      +84     
- Misses        235      357     +122
Impacted Files Coverage Δ
inst/include/Rcpp/r_cast.h 100% <ø> (ø) ⬆️
inst/include/Rcpp/S4.h 70.58% <ø> (ø) ⬆️
inst/include/Rcpp/Function.h 80% <0%> (-8.89%) ⬇️
inst/include/Rcpp/proxy/SlotProxy.h 91.66% <0%> (ø) ⬆️
inst/include/Rcpp/vector/Vector.h 88.09% <100%> (+0.19%) ⬆️
inst/include/Rcpp/as.h 81.48% <28.57%> (-10.52%) ⬇️
inst/include/Rcpp/internal/export.h 87.5% <33.33%> (-12.5%) ⬇️
inst/include/Rcpp/XPtr.h 60.52% <33.33%> (-1.64%) ⬇️
inst/include/Rcpp/utils/tinyformat.h 41.91% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25c2369...dd926e4. Read the comment docs.

codecov-io commented Apr 19, 2017

Codecov Report

Merging #677 into master will decrease coverage by 3.05%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #677      +/-   ##
==========================================
- Coverage   92.88%   89.82%   -3.06%     
==========================================
  Files          65       66       +1     
  Lines        3303     3509     +206     
==========================================
+ Hits         3068     3152      +84     
- Misses        235      357     +122
Impacted Files Coverage Δ
inst/include/Rcpp/r_cast.h 100% <ø> (ø) ⬆️
inst/include/Rcpp/S4.h 70.58% <ø> (ø) ⬆️
inst/include/Rcpp/Function.h 80% <0%> (-8.89%) ⬇️
inst/include/Rcpp/proxy/SlotProxy.h 91.66% <0%> (ø) ⬆️
inst/include/Rcpp/vector/Vector.h 88.09% <100%> (+0.19%) ⬆️
inst/include/Rcpp/as.h 81.48% <28.57%> (-10.52%) ⬇️
inst/include/Rcpp/internal/export.h 87.5% <33.33%> (-12.5%) ⬇️
inst/include/Rcpp/XPtr.h 60.52% <33.33%> (-1.64%) ⬇️
inst/include/Rcpp/utils/tinyformat.h 41.91% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25c2369...dd926e4. Read the comment docs.

@kevinushey

This comment has been minimized.

Show comment
Hide comment
@kevinushey

kevinushey Apr 19, 2017

Contributor

LGTM!

Contributor

kevinushey commented Apr 19, 2017

LGTM!

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 19, 2017

Member

Update: Not quite half-way done in the reverse-depends check and looking good just like the others did.

Member

eddelbuettel commented Apr 19, 2017

Update: Not quite half-way done in the reverse-depends check and looking good just like the others did.

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Apr 20, 2017

Owner

Made a slight boo boo. Should be %i instead of %s here

Owner

coatless commented on inst/include/Rcpp/api/meat/DottedPairImpl.h in 29f28c4 Apr 20, 2017

Made a slight boo boo. Should be %i instead of %s here

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 20, 2017

Collaborator

%i or %d?

Collaborator

eddelbuettel replied Apr 20, 2017

%i or %d?

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Apr 20, 2017

Owner

In this scenario, they are equivalent.

Owner

coatless replied Apr 20, 2017

In this scenario, they are equivalent.

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 20, 2017

Collaborator

Remind me when they differ. Somehow over decades of C(++) my fingers wired themselves to %d not %i.

Collaborator

eddelbuettel replied Apr 20, 2017

Remind me when they differ. Somehow over decades of C(++) my fingers wired themselves to %d not %i.

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Apr 20, 2017

Owner

Most likely from the days of using scanf()? %d assumes base 10 while %i detects the appropriate base.

Owner

coatless replied Apr 20, 2017

Most likely from the days of using scanf()? %d assumes base 10 while %i detects the appropriate base.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 20, 2017

Member

We got eight failures, which is up by two:

  • dplyr which is benign, it is checking against an exact string and now that this PR changes the format it comes up differently; given the dplyr release pipeline for next month I think we can get that adjusted, ccing @krlmlr
  • rstanarm which an obscure looking testthat error -- and given how we just ran multiple checks over the last few days that is a possible concern. Now that was 2.14.1 and they just release 2.15.2 yesterday and that passes 🥇
    So I think we're good with this one as well.
Member

eddelbuettel commented Apr 20, 2017

We got eight failures, which is up by two:

  • dplyr which is benign, it is checking against an exact string and now that this PR changes the format it comes up differently; given the dplyr release pipeline for next month I think we can get that adjusted, ccing @krlmlr
  • rstanarm which an obscure looking testthat error -- and given how we just ran multiple checks over the last few days that is a possible concern. Now that was 2.14.1 and they just release 2.15.2 yesterday and that passes 🥇
    So I think we're good with this one as well.
@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 20, 2017

Member

@krlmlr : What I got was for the (CRAN version of) dplyr:

* checking tests ... ERROR
  Running ‘testthat.R’
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  Actual value: "Expecting a single value: [extent=0]."
  
  
  2. Failure: summarise gives proper errors (#153) (@test-summarise.r#62) --------
  error$message does not match "expecting a single value".
  Actual value: "Expecting a single value: [extent=2]."
  
  
  testthat results ================================================================
  OK: 1507 SKIPPED: 1 FAILED: 2
  1. Failure: summarise gives proper errors (#153) (@test-summarise.r#61) 
  2. Failure: summarise gives proper errors (#153) (@test-summarise.r#62) 
  
  Error: testthat unit tests failed
  Execution halted
* checking for unstated dependencies in vignettes ... OK

Would it be possible to soften that test to accept either the old or the new string?

Member

eddelbuettel commented Apr 20, 2017

@krlmlr : What I got was for the (CRAN version of) dplyr:

* checking tests ... ERROR
  Running ‘testthat.R’
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  Actual value: "Expecting a single value: [extent=0]."
  
  
  2. Failure: summarise gives proper errors (#153) (@test-summarise.r#62) --------
  error$message does not match "expecting a single value".
  Actual value: "Expecting a single value: [extent=2]."
  
  
  testthat results ================================================================
  OK: 1507 SKIPPED: 1 FAILED: 2
  1. Failure: summarise gives proper errors (#153) (@test-summarise.r#61) 
  2. Failure: summarise gives proper errors (#153) (@test-summarise.r#62) 
  
  Error: testthat unit tests failed
  Execution halted
* checking for unstated dependencies in vignettes ... OK

Would it be possible to soften that test to accept either the old or the new string?

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Apr 20, 2017

Contributor

This test seems to have changed completely in the dev version of dplyr.

Contributor

krlmlr commented Apr 20, 2017

This test seems to have changed completely in the dev version of dplyr.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 20, 2017

Member

Ah. Silly me. Could have tested myself. Is there a tarball somewhere I should fetch?

Member

eddelbuettel commented Apr 20, 2017

Ah. Silly me. Could have tested myself. Is there a tarball somewhere I should fetch?

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Apr 20, 2017

Contributor

We're not creating tarballs automatically, sorry. Building from a clone works for me, but you'd also need the dev version of rlang.

Contributor

krlmlr commented Apr 20, 2017

We're not creating tarballs automatically, sorry. Building from a clone works for me, but you'd also need the dev version of rlang.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 20, 2017

Member

That's puts it a little outside my test framework. I'll merge this, maybe you can do me the favour of testing against the master branch of Rcpp one of these days.

Member

eddelbuettel commented Apr 20, 2017

That's puts it a little outside my test framework. I'll merge this, maybe you can do me the favour of testing against the master branch of Rcpp one of these days.

@eddelbuettel eddelbuettel merged commit 8f2a101 into RcppCore:master Apr 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Apr 20, 2017

Contributor

Sure. Note: I'm getting lots of warning: extra ‘;’ [-Wpedantic] when compiling 4e300ae.

Contributor

krlmlr commented Apr 20, 2017

Sure. Note: I'm getting lots of warning: extra ‘;’ [-Wpedantic] when compiling 4e300ae.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 20, 2017

Member

All quiet here with -fpic -g -O3 -Wall -pipe -pedantic -Wextra -Wno-empty-body -Wno-unused (where I had I had to add some of the -Wno... for either at-work code or other projects).

Member

eddelbuettel commented Apr 20, 2017

All quiet here with -fpic -g -O3 -Wall -pipe -pedantic -Wextra -Wno-empty-body -Wno-unused (where I had I had to add some of the -Wno... for either at-work code or other projects).

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Apr 20, 2017

Contributor

Just ran it, tests break everywhere :-(

We did just add strict checks of the wording of error messages. How do we avoid Rcpp prepending/appending stuff to our error messages?

Contributor

krlmlr commented Apr 20, 2017

Just ran it, tests break everywhere :-(

We did just add strict checks of the wording of error messages. How do we avoid Rcpp prepending/appending stuff to our error messages?

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Apr 20, 2017

Contributor
> R CMD INSTALL .
* installing to library ‘/home/muelleki/R/x86_64-pc-linux-gnu-library/3.3’
* installing *source* package ‘Rcpp’ ...
** libs
g++ -std=c++98 -I/usr/share/R/include -DNDEBUG -I../inst/include/     -Wall -pedantic  -fpic  -g -O2 -fdebug-prefix-map=/build/r-base-uDrdxB/r-base-3.3.3=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g  -c barrier.cpp -o barrier.o
g++ -std=c++98 -I/usr/share/R/include -DNDEBUG -I../inst/include/     -Wall -pedantic  -fpic  -g -O2 -fdebug-prefix-map=/build/r-base-uDrdxB/r-base-3.3.3=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g  -c Rcpp_init.cpp -o Rcpp_init.o
In file included from ../inst/include/RcppCommon.h:122:0,
                 from ../inst/include/Rcpp.h:27,
                 from Rcpp_init.cpp:24:
../inst/include/Rcpp/exceptions.h:168:379: warning: extra ‘;’ [-Wpedantic]
     RCPP_ADVANCED_EXCEPTION_CLASS(not_compatible, "Not compatible" )
                                                                                                                                                                                                                                                                                                                                                                                           ^
In file included from ../inst/include/RcppCommon.h:122:0,
                 from ../inst/include/Rcpp.h:27,
                 from Rcpp_init.cpp:24:
../inst/include/Rcpp/exceptions.h:172:22: warning: extra ‘;’ [-Wpedantic]
     #undef RCPP_EXCEPTION_CLASS
                      ^
../inst/include/Rcpp/exceptions.h:173:84: warning: extra ‘;’ [-Wpedantic]
     #undef RCPP_ADVANCED_EXCEPTION_CLASS
                                                                                    ^
../inst/include/Rcpp/exceptions.h:175:53: warning: extra ‘;’ [-Wpedantic]

[...snip...]

** building package indices
** installing vignettes
** testing if installed package can be loaded
* DONE (Rcpp)

Did I miss a step for building Rcpp?

Contributor

krlmlr commented Apr 20, 2017

> R CMD INSTALL .
* installing to library ‘/home/muelleki/R/x86_64-pc-linux-gnu-library/3.3’
* installing *source* package ‘Rcpp’ ...
** libs
g++ -std=c++98 -I/usr/share/R/include -DNDEBUG -I../inst/include/     -Wall -pedantic  -fpic  -g -O2 -fdebug-prefix-map=/build/r-base-uDrdxB/r-base-3.3.3=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g  -c barrier.cpp -o barrier.o
g++ -std=c++98 -I/usr/share/R/include -DNDEBUG -I../inst/include/     -Wall -pedantic  -fpic  -g -O2 -fdebug-prefix-map=/build/r-base-uDrdxB/r-base-3.3.3=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -g  -c Rcpp_init.cpp -o Rcpp_init.o
In file included from ../inst/include/RcppCommon.h:122:0,
                 from ../inst/include/Rcpp.h:27,
                 from Rcpp_init.cpp:24:
../inst/include/Rcpp/exceptions.h:168:379: warning: extra ‘;’ [-Wpedantic]
     RCPP_ADVANCED_EXCEPTION_CLASS(not_compatible, "Not compatible" )
                                                                                                                                                                                                                                                                                                                                                                                           ^
In file included from ../inst/include/RcppCommon.h:122:0,
                 from ../inst/include/Rcpp.h:27,
                 from Rcpp_init.cpp:24:
../inst/include/Rcpp/exceptions.h:172:22: warning: extra ‘;’ [-Wpedantic]
     #undef RCPP_EXCEPTION_CLASS
                      ^
../inst/include/Rcpp/exceptions.h:173:84: warning: extra ‘;’ [-Wpedantic]
     #undef RCPP_ADVANCED_EXCEPTION_CLASS
                                                                                    ^
../inst/include/Rcpp/exceptions.h:175:53: warning: extra ‘;’ [-Wpedantic]

[...snip...]

** building package indices
** installing vignettes
** testing if installed package can be loaded
* DONE (Rcpp)

Did I miss a step for building Rcpp?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 20, 2017

Member

I truncated that a little. No need to flood all our screens.

I don't know and cannot know what you did. You probably used devtools; I don't. I upgrade the repo to current, built a tarball and installed that. Works for me.

Member

eddelbuettel commented Apr 20, 2017

I truncated that a little. No need to flood all our screens.

I don't know and cannot know what you did. You probably used devtools; I don't. I upgrade the repo to current, built a tarball and installed that. Works for me.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 20, 2017

Member

As for

We did just add strict checks of the wording of error messages. How do we avoid Rcpp prepending/appending stuff to our error messages?

I suggest to contract with @coatless who is the guilty party^Hunsung hero who improved our messages. Maybe he is motivated to help you update yours.

Member

eddelbuettel commented Apr 20, 2017

As for

We did just add strict checks of the wording of error messages. How do we avoid Rcpp prepending/appending stuff to our error messages?

I suggest to contract with @coatless who is the guilty party^Hunsung hero who improved our messages. Maybe he is motivated to help you update yours.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 20, 2017

Member

The warnings above look very much like something we had recently but cleaned since. Is it all possible that you are not at a clean checkout of the master branch?

Member

eddelbuettel commented Apr 20, 2017

The warnings above look very much like something we had recently but cleaned since. Is it all possible that you are not at a clean checkout of the master branch?

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Apr 20, 2017

Contributor

@eddelbuettel: I addressed the warning: extra ‘;’ [-Wpedantic] change I could find in the cpp11 macro. It seems as if @krlmlr is getting them in the cpp98 macro, which I can't replicate. Could you give an OS + compiler version?

RE: Exception message overide: Unfortunately @krlmlr, when I designed the new exceptions I tried to remove the appending of specific phrases in the file. Thus, the new ctors are all setup to either give "Message." or "Message: [Debug Info]".

I'm more than happy to spend some cycles correcting the unit tests in dplyr sometime this weekend/next week. (I think there are only two?)

Contributor

coatless commented Apr 20, 2017

@eddelbuettel: I addressed the warning: extra ‘;’ [-Wpedantic] change I could find in the cpp11 macro. It seems as if @krlmlr is getting them in the cpp98 macro, which I can't replicate. Could you give an OS + compiler version?

RE: Exception message overide: Unfortunately @krlmlr, when I designed the new exceptions I tried to remove the appending of specific phrases in the file. Thus, the new ctors are all setup to either give "Message." or "Message: [Debug Info]".

I'm more than happy to spend some cycles correcting the unit tests in dplyr sometime this weekend/next week. (I think there are only two?)

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 21, 2017

Member

I got this -- just a matter of imposing -std=c++98 in ~/.R/Makevars

Member

eddelbuettel commented Apr 21, 2017

I got this -- just a matter of imposing -std=c++98 in ~/.R/Makevars

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Apr 21, 2017

Contributor

I indeed have -std=c++98 in ~/.R/Makevars. I thought my build process, and the compiler settings, were obvious from my output. Sorry for flooding the screen -- I'm a happy user of Octopatcher (for Firefox, Chrome, and some other browser), which takes care of collapsing long code blocks.

@coatless: I'd rather prefer full control over the error messages. Would that be very difficult to implement on top of your extensions, which I'm sure are useful in most other cases? Obligatory: xkcd: Workflow

Contributor

krlmlr commented Apr 21, 2017

I indeed have -std=c++98 in ~/.R/Makevars. I thought my build process, and the compiler settings, were obvious from my output. Sorry for flooding the screen -- I'm a happy user of Octopatcher (for Firefox, Chrome, and some other browser), which takes care of collapsing long code blocks.

@coatless: I'd rather prefer full control over the error messages. Would that be very difficult to implement on top of your extensions, which I'm sure are useful in most other cases? Obligatory: xkcd: Workflow

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Apr 22, 2017

Contributor

@krlmlr

To clarify, you do have full control over the error messages being triggered by Rcpp::stop() and Rcpp::warning() just as before.

Furthermore, you gain even more control over exceptions that have been upgraded to using RCPP_ADVANCED_EXCEPTION_CLASS, which permits a printf() syntax similar to what is available in Rcpp::stop() and Rcpp::warning(). These exceptions are: not_compatible and index_out_of_bounds. If you do not opt to use this method by supplying a const char* followed by individual parameters, then ctors from the old style exceptions come into play. These ctors were traditional in the form of: "message" or "message: user msg". To be more inline with exception messages added in, these have been updated to match "Message." or "Message: user msg." to emphasize what the error was and that it terminates/ends.

With this being said, the prior exception classes, RCPP_SIMPLE_EXCEPTION_CLASS and RCPP_EXCEPTION_CLASS, have traditionally been restricted to internally defined strings. The later of said classes only providing support for an addendum to a prefixed message. To support a subtle upgrade, a new default ctor was added into RCPP_EXCEPTION_CLASS to support "Message."

Overall, these changes were made to improve the experience developers have while working with Rcpp. In particular, the need for such changes originated based on my experience observing students in courses that I've created running into difficulty understanding what these exception messages mean.

Therefore, I'm more of a proponent of working to fix the unit tests within dplyr for two reason:

  1. This esoteric code will likely never be touched again for some time and when it is @eddelbuettel will construct a gauntlet such that no reasonable doubt will exist in the aforementioned change.
    • There was only 1 previous overhaul of exceptions when tinyformat was introduced in 2014.
    • Prior to that, the exception model remained relatively intact circa 2010.
  2. Based on the reverse dependence, only dplyr is running into this issue.
Contributor

coatless commented Apr 22, 2017

@krlmlr

To clarify, you do have full control over the error messages being triggered by Rcpp::stop() and Rcpp::warning() just as before.

Furthermore, you gain even more control over exceptions that have been upgraded to using RCPP_ADVANCED_EXCEPTION_CLASS, which permits a printf() syntax similar to what is available in Rcpp::stop() and Rcpp::warning(). These exceptions are: not_compatible and index_out_of_bounds. If you do not opt to use this method by supplying a const char* followed by individual parameters, then ctors from the old style exceptions come into play. These ctors were traditional in the form of: "message" or "message: user msg". To be more inline with exception messages added in, these have been updated to match "Message." or "Message: user msg." to emphasize what the error was and that it terminates/ends.

With this being said, the prior exception classes, RCPP_SIMPLE_EXCEPTION_CLASS and RCPP_EXCEPTION_CLASS, have traditionally been restricted to internally defined strings. The later of said classes only providing support for an addendum to a prefixed message. To support a subtle upgrade, a new default ctor was added into RCPP_EXCEPTION_CLASS to support "Message."

Overall, these changes were made to improve the experience developers have while working with Rcpp. In particular, the need for such changes originated based on my experience observing students in courses that I've created running into difficulty understanding what these exception messages mean.

Therefore, I'm more of a proponent of working to fix the unit tests within dplyr for two reason:

  1. This esoteric code will likely never be touched again for some time and when it is @eddelbuettel will construct a gauntlet such that no reasonable doubt will exist in the aforementioned change.
    • There was only 1 previous overhaul of exceptions when tinyformat was introduced in 2014.
    • Prior to that, the exception model remained relatively intact circa 2010.
  2. Based on the reverse dependence, only dplyr is running into this issue.

@coatless coatless deleted the coatless:feature/improved-exception-msgs branch Apr 22, 2017

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Apr 23, 2017

Contributor

One additional exception message change was added in:

7492cff

Contributor

coatless commented Apr 23, 2017

One additional exception message change was added in:

7492cff

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Apr 23, 2017

Contributor

@coatless: Thanks I forgot that we are using R code to construct and throw most error messages, even those that are detected at the C++ level. This is for both simplicity and consistency. We could avoid throwing the messages from R and throw from C++, but I'd rather catch in C++ and re-throw, again for the same reasons. Please advise.

Contributor

krlmlr commented Apr 23, 2017

@coatless: Thanks I forgot that we are using R code to construct and throw most error messages, even those that are detected at the C++ level. This is for both simplicity and consistency. We could avoid throwing the messages from R and throw from C++, but I'd rather catch in C++ and re-throw, again for the same reasons. Please advise.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 23, 2017

Member

Unless I am missing something, both halves of those broken eggs originate with you: you set up the message you write, you catch the message and in the testing framework very stringently compare them.

As @coatless pointed out, we don't tend to change these things all that often, but for whatever reason he caught me at a weak moment and I let it pass.

So for once you (at the downstream package using it) need to update the comparison texts you are getting. We are pretty serious about keeping APIs stable so this particular problem should not hit you again in the foreseeable future.

Member

eddelbuettel commented Apr 23, 2017

Unless I am missing something, both halves of those broken eggs originate with you: you set up the message you write, you catch the message and in the testing framework very stringently compare them.

As @coatless pointed out, we don't tend to change these things all that often, but for whatever reason he caught me at a weak moment and I let it pass.

So for once you (at the downstream package using it) need to update the comparison texts you are getting. We are pretty serious about keeping APIs stable so this particular problem should not hit you again in the foreseeable future.

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Apr 24, 2017

Contributor

@krlmlr I've opened up an issue ticket on tidyverse/dplyr#2702.

Contributor

coatless commented Apr 24, 2017

@krlmlr I've opened up an issue ticket on tidyverse/dplyr#2702.

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Apr 24, 2017

Contributor

@eddelbuettel: I'm throwing messages in R, called from C++ code. The R <- C++ <- R roundtrip now adds prefix+suffix to the messages, which is useful in most cases but not in our very peculiar situation. We'll be working on a way to avoid that. This looks feasible because we call back into R to generate the error message only at very few designated places, we only need a way to extract the original message without prefix+suffix from the exception.

Contributor

krlmlr commented Apr 24, 2017

@eddelbuettel: I'm throwing messages in R, called from C++ code. The R <- C++ <- R roundtrip now adds prefix+suffix to the messages, which is useful in most cases but not in our very peculiar situation. We'll be working on a way to avoid that. This looks feasible because we call back into R to generate the error message only at very few designated places, we only need a way to extract the original message without prefix+suffix from the exception.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment