-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Remove events #550
Conversation
python -m unittest tests.test_integration | ||
python -m unittest tests.test_resources | ||
python -m unittest tests.test_configs | ||
./test.sh |
There was a problem hiding this comment.
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'] |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
That's been merged - at this point I believe we can simply make a release of |
|
||
return events | ||
if 'dashEvents' in props or 'fireEvents' in props: | ||
raise AttributeError('Events are no longer supported by dash') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
echo "All tests passed!" | ||
fi | ||
|
||
exit $EXIT_STATE |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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.
Closes #531
Dash complement to plotly/dash-renderer#114 - note that the
no-events
branch ofdash-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 usenpm
, so I made a shell scripttest.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