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

Make NoDefaultSpecified and no_default_specified private #1380

Merged
merged 3 commits into from
Jan 4, 2021

Conversation

kitchoi
Copy link
Contributor

@kitchoi kitchoi commented Jan 4, 2021

Closes #1379

This PR fixes the issue by making NoDefaultSpecified and no_default_specified private by naming convention. We believe they are not (actively) used outside of traits. If they are indeed used, this PR will at least make those breakage fail in a more obvious and trackable fashion, and we will get to know why and how it gets used, and then we can fix the breakage accordingly.

Note that if I change this line to if True::

if default_value is not no_default_specified:

I would get a test failure for the test that involves TraitsUI:

======================================================================
ERROR: test_create_editor (traits.tests.test_enum.TestGui)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/kchoi/Work/ETS/traits/traits/tests/test_enum.py", line 320, in test_create_editor
    with UITester().create_ui(obj):
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/contextlib.py", line 117, in __enter__
    return next(self.gen)
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traitsui/testing/tester/ui_tester.py", line 88, in create_ui
    ui = object.edit_traits(**ui_kwargs)
  File "/Users/kchoi/Work/ETS/traits/traits/has_traits.py", line 1736, in edit_traits
    return view.ui(
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traitsui/view.py", line 462, in ui
    ui.ui(parent, kind)
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traitsui/ui.py", line 244, in ui
    self.rebuild(self, parent)
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traitsui/qt4/toolkit.py", line 163, in ui_live
    ui_live.ui_live(ui, parent)
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traitsui/qt4/ui_live.py", line 43, in ui_live
    _ui_dialog(ui, parent, BaseDialog.NONMODAL)
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traitsui/qt4/ui_live.py", line 65, in _ui_dialog
    BaseDialog.display_ui(ui, parent, style)
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traitsui/qt4/ui_base.py", line 278, in display_ui
    ui.owner.init(ui, parent, style)
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traitsui/qt4/ui_live.py", line 225, in init
    self.add_contents(panel(ui), bbox)
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traitsui/qt4/ui_panel.py", line 253, in panel
    content = ui._groups
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traitsui/ui.py", line 832, in _get__groups
    shadow_group = self.view.content.get_shadow(self)
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traitsui/group.py", line 288, in get_shadow
    content.append(value.get_shadow(ui))
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traitsui/group.py", line 297, in get_shadow
    return ShadowGroup(shadow=self, content=content, groups=groups)
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traitsui/group.py", line 546, in __init__
    self.shadow = shadow
traits.trait_errors.TraitError: Cannot modify the read only 'shadow' attribute of a 'ShadowGroup' object.

Instead of relying on this test which really belonged to TraitsUI, I added another one that would fail for the same reason.

Ideally there should be a test for TraitType.clone, but I struggle to understand how it is supposed to be used. From what I saw, it is predominantly used by TraitType.__call__ and _TraitMaker. The former is used in the metaclass and the latter is used by Trait... both are quite complex. Eventually I decided this can be an exercise for another day, the existing usage in TraitsUI deserves a test in Traits anyway and it would provide the protection we needed for the code we changed here.

Checklist

  • Tests
  • Update API reference (docs/source/traits_api_reference)
  • Update User manual (docs/source/traits_user_manual) : This NoDefaultSpecified is not mentioned in the user manual
  • Update type annotation hints in traits-stubs

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@mdickinson
Copy link
Member

If you have a moment, it may make sense to add a changelog entry right now so that we don't forget later. But it can also wait until later.

@kitchoi
Copy link
Contributor Author

kitchoi commented Jan 4, 2021

Yes it would make sense to update changelog together with the PR. I tentatively categorize this as a removal, because it is a removal from the public API.

Since we have not got the news fragments machinery here, I updated the changelog directly instead. I am slightly worried about merge conflicts, but it is typically trivial enough (albeit a bit annoying at times).

@mdickinson
Copy link
Member

Yes, we'll deal with any merge conflicts.

Thank you!

@mdickinson mdickinson merged commit 2a3d68b into master Jan 4, 2021
@mdickinson mdickinson deleted the 1379-make-no-default-specified-private branch January 4, 2021 14:53
@mdickinson mdickinson mentioned this pull request Jan 7, 2021
4 tasks
mdickinson added a commit that referenced this pull request Jan 12, 2021
* Revert PR #1380, but keep the test in test_readonly.py

* Revert "Prevent singleton instance from name shadowing its class (#1378)"

This reverts commit a68bc50.

* Rename the type, add docstrings for the type and the singleton instance

* Fix the doc build

* Add tests for NoDefaultSpecified

* Add paragraph explaining the intended purpose of NoDefaultSpecified

* Remove misleading typing stub for NoDefaultSpecified (which isn't a class)

* Fix spurious blank line change
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.

Resolve how NoDefaultSpecified singleton or class should be exposed and documented
2 participants