-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
prevent the creation of new px.default properties at run time #2183
Conversation
Very elegant solution! I didn't know |
I have added a test- thanks for the reminder. I will try to proactively add them where relevant going forward :) |
@@ -232,3 +233,9 @@ def assert_orderings(days_order, days_check, times_order, times_check): | |||
for days in permutations(df["day"].unique()): | |||
for times in permutations(df["time"].unique()): | |||
assert_orderings(days, days, times, times) | |||
|
|||
|
|||
def check_permissive_defaults(): |
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 your function should be called test_something
so that pytest catches it in the test suite (otherwise it will consider it a helper function). You can verify this by executing locally pytest in your machine (for example in the same directory, pytest -v test_px.py
and check whether the new function appears). Maybe I'm wrong but please check :-).
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.
You're right about tests needing to start with test
in order to be picked up by pytest
.
After modifying to above code, the output of running pytest -v test_px.py
is:
test_px.py::test_scatter PASSED
test_px.py::test_custom_data_scatter PASSED
test_px.py::test_labels PASSED
test_px.py::test_px_templates PASSED
test_px.py::test_orthogonal_orderings PASSED
test_px.py::test_permissive_defaults PASSED
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.
awesome!
Thanks @jdamiba ! Just wait for the CI to finish (everything should be green except for one of the orca builds which failure is not related to this PR), and then 💃 ! |
closes #2168
The purpose of this PR is to prevent the creation of new px.default properties at run time.
Testing Plan:
Prior to this PR, the following code does not throw an exception:
When running this PR, the above code throws: