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

feat: Raise warning if reactive.Effect function returns a value #427

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wch
Copy link
Collaborator

@wch wch commented Mar 18, 2023

Closes #425.
If a user function passed to reactive.Effect returns a value, that is most likely a mistake. This PR causes Shiny to raise a warning in that case. For example:

@reactive.Effect
def _():
    return 1

@wch
Copy link
Collaborator Author

wch commented Mar 18, 2023

Note that the warning message should be improved to print the line number in the user code where the return happened. This implementation simply causes Python to print out the source where the warning is raised, which always in the same place in _reactives.py.

await reactive.flush()
/Users/winston/py-shiny/shiny/reactive/_reactives.py:516: ReactiveWarning: reactive.Effect function returned a value, but should return None; reactive.Effects should only be used for side effects. Did you mean to use reactive.Calc instead?
  await self._run()

"Did you mean to use reactive.Calc instead?",
ReactiveWarning,
stacklevel=2,
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this PR--can you just tell me if the warning here gives any helpful indication as to which reactive.Effect is causing the problem? If not, we could use inspect.unwrap(foobar._fn).__code__.co_filename and .co_firstlineno

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, the problem is that we don't know how many times the user-defined function has been wrapped, so we don't know how many levels in the stack we need to skip.

For example, if the user function is synchronous, then we wrap it once in an async function. But they may decorated it with @reactive.event, which would add another layer.

Now, if the user function threw an exception, I think we would be able to catch that and find the source of it, but that's not the situation here.

@nealrichardson
Copy link
Collaborator

@jcheng5 @wch is this good to merge?

@nealrichardson nealrichardson changed the title Raise warning if reactive.Effect function returns a value feat: Raise warning if reactive.Effect function returns a value Jun 5, 2023
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.

reactive.Effect should warn if function returns a value
3 participants