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

Pick up app name from server instance (#771) #774

Merged
merged 4 commits into from
Jun 16, 2019

Conversation

Batalex
Copy link
Contributor

@Batalex Batalex commented Jun 15, 2019

Start with a description of this PR. Then edit the list below to the items that make sense for your PR scope, and check off the boxes as you go!

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
  • 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

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follow
    • this github #PR number updates the dash docs
    • here is the show and tell thread in plotly dash community

Details

I changed the default name arg value from __main__ to None in order to distinguish "not passing a name" from "setting explicitly the name to __main__ ". I figured it would be the best course of action to avoid unwanted renaming cases.

dash/dash.py Outdated Show resolved Hide resolved
@alexcjohnson
Copy link
Collaborator

Looks great, thanks! Would you mind adding a test, perhaps in https://github.com/plotly/dash/blob/master/tests/unit/test_configs.py, that just instantiates an app in various ways and checks that app.config.name has the desired value?

@alexcjohnson
Copy link
Collaborator

I changed the default name arg value from __main__ to None in order to distinguish "not passing a name" from "setting explicitly the name to __main__ ". I figured it would be the best course of action to avoid unwanted renaming cases.

💯 I don't know why a user would do this, but it should definitely behave the way you've done it!

@Batalex
Copy link
Contributor Author

Batalex commented Jun 15, 2019

@alexcjohnson I see that https://github.com/plotly/dash/blob/master/tests/unit/test_configs.py still use unitest, am I free to import pytest? pytest.mark.parametrize would be useful for this PR -> multiple cases to test

@alexcjohnson
Copy link
Collaborator

Yes, you are more than welcome to use pytest! We'll be converting this and the others the rest of the way soon, just haven't gotten to this file yet.

@Batalex
Copy link
Contributor Author

Batalex commented Jun 16, 2019

It should be good now. To keep a narrow scope for this PR I added the bare minimum for the test but I would be glad to continue this work in a new PR with a full convertion to pytest

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.

Beautiful test, thanks! 💃

And yes, you're right that it's best not to mix in other topics like converting more tests to pytest into this PR. But if you're interested in helping that process in another PR we would love to have your contribution! If you haven't seen it, https://github.com/plotly/dash-docs/pull/520 describes the new pytest fixtures we (@byronz) made, which are in master here but have not yet been released.

@alexcjohnson alexcjohnson merged commit 50cbc22 into plotly:master Jun 16, 2019
@Batalex Batalex mentioned this pull request Jun 18, 2019
3 tasks
HammadTheOne added a commit to HammadTheOne/dash that referenced this pull request May 28, 2021
* Added conditionally set props

* Updated changelog.

* Added integration test to check callbacks
HammadTheOne added a commit that referenced this pull request Jul 23, 2021
* Added conditionally set props

* Updated changelog.

* Added integration test to check callbacks
@Batalex Batalex deleted the feat/pick-server-name branch December 16, 2021 15:39
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

2 participants