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

Fixed bug that prevented env DASH_DEBUG to work #2047

Merged
merged 7 commits into from
May 21, 2022

Conversation

PGrawe
Copy link
Contributor

@PGrawe PGrawe commented May 12, 2022

Hey,
setting the debug value with the environment variable DASH_DEBUG=true, didn't work.
The default, if no debug value is given, is still false. Everything else works as intended.
This also fixes #1979 .

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • Changing the default debug value for app.run to None
    • Changing the default debug value to False in enable_dev_tools
  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

@PGrawe PGrawe requested a review from alexcjohnson as a code owner May 12, 2022 07:29
@alexcjohnson
Copy link
Collaborator

Thanks @PGrawe! There's a little more going on though - I suspect this PR breaks the documented behavior of app.enable_dev_tools:

dash/dash/dash.py

Lines 1653 to 1657 in b84fad8

:param debug: Enable/disable all the dev tools unless overridden by the
arguments or environment variables. Default is ``True`` when
``enable_dev_tools`` is called directly, and ``False`` when called
via ``run``. env: ``DASH_DEBUG``
:type debug: bool

So maybe we need two calls to get_combined_config - put the one inside enable_dev_tools back to the way it was, but add another in run that defaults to False?

Now, how can we test this behavior? test_configs.py seems like the right place and has a lot of similar examples, and ideally we should test ten cases: (app.enable_dev_tools or app.run) x (DASH_DEBUG=true | false, or no env var with debug=True | False | None in the method call). The debug value isn't explicitly stored, but you can look at for example app._dev_tools.ui which should have the same value as debug if you haven't done anything else special.

Testing app.run in a unit test is tricky because it kicks off the blocking server run at the end. But I bet we could hack around this by getting it to throw an error after its internal enable_dev_tools call - for example app.run(debug=debug, port=-1) would throw here so you could do something like:

with pytest.raises(AssertionError):
    app.run(debug=debug, port=-1)
assert app._dev_tools.ui == expected_debug

@PGrawe
Copy link
Contributor Author

PGrawe commented May 18, 2022

Hey Alex,
thank you so much for your help! I really appreciate it. 🙏
I changed everything that you suggested and implemented the tests and they passed.
Let me know what you think.

EDIT: Still got some linting error. I'll fix that tomorrow!

  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Excellent! Can you add a changelog entry? Then we'll merge 🎉

@PGrawe
Copy link
Contributor Author

PGrawe commented May 21, 2022

Amazing! 💯 Just updated the changelog. Thanks for all your help 😃

@alexcjohnson alexcjohnson merged commit b59ac20 into plotly:dev May 21, 2022
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.

DASH_DEBUG variable has no effect
2 participants