Skip to content
New issue

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

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

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # 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