Skip to content
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

Experiment with error message #3007

Merged
merged 8 commits into from Dec 4, 2020
Merged

Experiment with error message #3007

merged 8 commits into from Dec 4, 2020

Conversation

hadley
Copy link
Member

@hadley hadley commented Aug 18, 2020

Created for the purposes of discussion.

Before:

Error in .getReactiveEnvironment()$currentContext() : 
  Operation not allowed without an active reactive context. (You tried to do something that can only be done from inside a reactive expression or observer.)

After:

Error: Operation not allowed without an active reactive context.
* You tried to do something that can only be done from inside a reactive consumer.

Also worth consider if it would be useful to include a hint here, or if that's ultimately too misleading:

Error: Operation not allowed without an active reactive context.
* You tried to do something that can only be done from inside a reactive consumer.
i Do you need to wrap the expression in `isolate()`?

@wch
Copy link
Collaborator

wch commented Aug 18, 2020

Looks reasonable to me. I think suggesting isolate() could point people to write bad code and not understand what it's doing.

@wch wch self-requested a review August 18, 2020 21:27
@hadley
Copy link
Member Author

hadley commented Aug 18, 2020

You can merge this if you want, or leave it open and I'll collect a few more error messages.

@wch
Copy link
Collaborator

wch commented Aug 19, 2020

I'll leave it open for now - let me know when you feel like it's done.

@cpsievert cpsievert added this to the 1.5.1 milestone Aug 19, 2020
@hadley
Copy link
Member Author

hadley commented Oct 17, 2020

This is worth another review — I've updated all errors in react.R and reactive.R following http://style.tidyverse.org/error-messages.html

@hadley
Copy link
Member Author

hadley commented Oct 19, 2020

Discovered a few older issues that I filed with suggestions for improved error mesages, so I tweaked those too.

@cpsievert cpsievert modified the milestones: 1.5.1, 1.6 Oct 30, 2020
@wch wch merged commit f169792 into rstudio:master Dec 4, 2020
cpsievert added a commit that referenced this pull request Dec 4, 2020
schloerke added a commit that referenced this pull request Dec 11, 2020
* master: (35 commits)
  Update NEWS
  Rebuild JS files
  Set min/max date before setting value. Closes #3197
  Respect shiny.minified for bootstrap-datepicker.js
  Skip POSIXlt slider tests on R3.6 and below
  Update NEWS
  validate_session_object: Also work with modules
  Fix test
  All session parameters from the update* functions now default to `getDefaultReactiveDomain()` (#3195)
  Respect reactiveConsole() in new errors (#3193)
  Add NEWS notes for #3042 and #3038 (#3191)
  Add a warning message when value < min | value > max in sliderInput (#3194)
  Clear selected date if not within min/max range (#3188)
  Add 'auto' brush fill and stroke (#2864)
  getCurrentOutputInfo() bugfix (#3189)
  radioButtons() and checkboxGroup() accessibility (#3187)
  Cleaner logic for conditional CSS styles (#2671)
  Experiment with error message (#3007)
  Build JS files
  Better setting of bootstrap-datepicker start/end dates, closes #2703
  ...
@daattali
Copy link
Contributor

Just noticed the new error messages! It's nice that very old messages from version 0.0.1 are being reexamined :)

Regarding the change of "reactive expression or observer" -> "reactive consumer", was that motivated by people being confused by the original message, or is it an attempt to be more "correct"? From talking to many intermediate shiny developers, most don't seem to know what "reactive consumer" means and I think this message might cause more confusion than it fixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants