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

Remove events #550

Merged
merged 12 commits into from
Jan 25, 2019
Merged

Remove events #550

merged 12 commits into from
Jan 25, 2019

Conversation

alexcjohnson
Copy link
Collaborator

@alexcjohnson alexcjohnson commented Jan 20, 2019

Closes #531

Dash complement to plotly/dash-renderer#114 - note that the no-events branch of dash-renderer is needed to get tests to pass here, but not vice versa.

In the other repos I've been making a single npm command to run all the tests, and using that command in ci. Here we don't use npm, so I made a shell script test.sh for the same purpose.

The component generation routines, as modified here, are what I used to rebuild the html components in the already-merged plotly/dash-html-components#89, and in a forthcoming PR in dcc

python -m unittest tests.test_integration
python -m unittest tests.test_resources
python -m unittest tests.test_configs
./test.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -122,7 +120,7 @@ def __repr__(self):
for p in prop_keys
if not p.endswith("-*") and
p not in python_keywords and
p not in ['dashEvents', 'fireEvent', 'setProps']] + ['**kwargs']
p != 'setProps'] + ['**kwargs']
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if that was intentional but still present for the R generation

Copy link
Contributor

@rpkyle rpkyle Jan 21, 2019

Choose a reason for hiding this comment

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

Have modified my code to match.

self.ComponentClass().available_events,
['restyle', 'relayout', 'click']
hasattr(self.ComponentClass(), 'available_events'),
False
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this test should be renamed to make clearer what it's testing now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe this test should be renamed

good call -> ec9aaee

@Marc-Andre-Rivet
Copy link
Contributor

This looks good to me

  • changelog entry missing
  • obviously need to point to master of dash-renderer after this

Would appreciate @T4rk1n or @rmarren1 input.

@alexcjohnson
Copy link
Collaborator Author

obviously need to point to master of dash-renderer after this

That's been merged - at this point I believe we can simply make a release of dash-renderer off master, and it will stop processing events even if used with older versions of everything else that still supports them; then we can make new releases of everything else (the only PR I haven't made yet is dcc, coming soon...) acknowledging and locking in the lack of event support. I believe the only incompatible combination will be new dash with old dash-renderer.


return events
if 'dashEvents' in props or 'fireEvents' in props:
raise AttributeError('Events are no longer supported by dash')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a custom exception or raise a DeprecationWarning instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, it wasn't deprecated, it was outright removed 😅 but I'm happy to make a custom exception - ObsoleteEventsError?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think exceptions.NonExistantPropException with a event removal message would fit the bill.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used NonExistentEventException, which is the same I use for corresponding obsolete callback usage.

test.sh Show resolved Hide resolved
echo "All tests passed!"
fi

exit $EXIT_STATE
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we include the linting also ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure! I'll move linting in here too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

linting into test.sh -> 2c9a3c8

Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

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

No python expert but this looks fine to me. 💃
Will take this out on a dash-docs run with the other dash repos before releasing, for sanity.

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.

4 participants