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

Use all python keywords when checking if prop name is valid #450

Merged
merged 4 commits into from
Nov 6, 2018

Conversation

rmarren1
Copy link
Contributor

@rmarren1 rmarren1 commented Nov 6, 2018

Solves #361

Previously, we did not allow a prop name to be written as a keyword argument in a Python Component class if it was in keyword.kwlist. Since keyword.kwlist is version specific, class files were dependent on the Python version used to create them.

This PR removes this problem, Python Component class files built with any of our supported versions (2.7, 3.3, 3.4, 3.5, 3.6, 3.7) should work for all the other ones too.

Note that these props are still usable, they are just passed through **kwargs rather than a proper keyword argument, so IDEs will not autocomplete for these props. async in html.Script is the only component in the core libraries affected by this so far.

@rmarren1 rmarren1 requested a review from T4rk1n November 6, 2018 00:03
@T4rk1n
Copy link
Contributor

T4rk1n commented Nov 6, 2018

I thought those tests failures were resolved by 9e3ca4d. Or are they new failures ?

@rmarren1
Copy link
Contributor Author

rmarren1 commented Nov 6, 2018

I'm not really sure, been trying to figure it out. I'm not getting those errors locally

@rmarren1
Copy link
Contributor Author

rmarren1 commented Nov 6, 2018

Locally I get this... strange

======================================================================
FAIL: test_serving_scripts (tests.test_react.IntegrationTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ryan/Plotly/dash/tests/test_react.py", line 331, in test_serving_scripts
    self.assertEqual(response.status_code, 200)
AssertionError: 404 != 200

----------------------------------------------------------------------

@T4rk1n
Copy link
Contributor

T4rk1n commented Nov 6, 2018

Fails for me too, that file is not tested on ci, maybe it's old ?

@rmarren1
Copy link
Contributor Author

rmarren1 commented Nov 6, 2018

Oh ok I'm using nose2 so it picked up that file. Looks like an old un-used test that was broken by a recent PR. Without it everything else works.

@T4rk1n
Copy link
Contributor

T4rk1n commented Nov 6, 2018

Try locking dash_core_components==0.35.1.

@rmarren1
Copy link
Contributor Author

rmarren1 commented Nov 6, 2018

Cool. Looks like dcc==0.37.0 was breaking

@T4rk1n
Copy link
Contributor

T4rk1n commented Nov 6, 2018

Yes, there is this bug plotly/dash-core-components#362, thought it was fixed in plotly/dash-core-components#356 but I guess it just made it worse. I have a test that fails in the conditions, I tried on one version of the commits and it passed. but after merge it failed again.

https://circleci.com/gh/plotly/dash-core-components/2022?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

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.

💃 I think we need those Input integration test in the dcc repo instead of here so the failures happens there instead.

@rmarren1 rmarren1 merged commit 1c94fd2 into master Nov 6, 2018
@rmarren1 rmarren1 deleted the remove-keywords branch November 6, 2018 01:07
AnnMarieW pushed a commit to AnnMarieW/dash that referenced this pull request Jan 6, 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.

2 participants