Skip to content

Move the dash.layout.setter so that type checkers can find it easier #3249

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

Merged

Conversation

peter-gerlagh-kyos
Copy link
Contributor

This rearranges two methods of the Dash class.
Specifically you now have @property def layout // @layout.setter def layout // def _layout_value instead of @property def layout // def _layout_value // @layout.setter def layout.
The point is to make it easier for type checkers (such as mypy) to connect the @property layout with the layout.setter, so that people who use this library with a type checker need fewer #type: ignore statements.
This is a very minimal change, and I don't see how it could break anything, or make anything worse. I don't think a note in the changelog is necessary.

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • change order of methods
    • confirm mypy is happy
    • run tests
  • 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 follows
    • this GitHub #PR number updates the dash docs
    • here is the show and tell thread in Plotly Dash community

@peter-gerlagh-kyos
Copy link
Contributor Author

I've not been able to run the test, and I don't understand the problem. dash_test_components seems to be missing (?)

@T4rk1n
Copy link
Contributor

T4rk1n commented Mar 26, 2025

I've not been able to run the test, and I don't understand the problem. dash_test_components seems to be missing (?)

For the you need to run npm run setup-tests.py, but it might takes pretty long, can just run the tests that fails the ci.

Copy link
Contributor

@T4rk1n T4rk1n left a comment

Choose a reason for hiding this comment

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

Look good, if that helps with linting doesn't hurt to change the order. 💃

@T4rk1n T4rk1n merged commit 28e8162 into plotly:dev Mar 27, 2025
3 checks passed
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.

2 participants