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

Fix documentation build due to class being name shadowed by a singleton #1378

Merged
merged 1 commit into from
Jan 4, 2021

Conversation

kitchoi
Copy link
Contributor

@kitchoi kitchoi commented Jan 4, 2021

Fix #1377

The singleton instance name is the same as its type name. This is confusing sphinx, and is causing the following error with Sphinx 3.4.1 while building doc:

reading sources... [ 51%] traits_api_reference/trait_type                                                                                                                                                                                 
Exception occurred:
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/sphinx/util/typing.py", line 160, in _restify_py37
    return ':obj:`%s.%s`' % (cls.__module__, cls.__name__)
AttributeError: 'NoDefaultSpecified' object has no attribute '__name__'
The full traceback has been saved in /var/folders/_6/bw6pnpwx3pzfvhd20ffcpyjm0000gn/T/sphinx-err-4260mu_5.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
make: *** [html] Error 2

Checklist

  • Tests
  • Update API reference (docs/source/traits_api_reference)
  • Update User manual (docs/source/traits_user_manual)
  • Update type annotation hints in traits-stubs

@kitchoi
Copy link
Contributor Author

kitchoi commented Jan 4, 2021

Alternatively we can change the class name to be different. Or maybe there are other tricks. However I find the name NoSpecifiedDefault misleading for an instance.

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, but let's make it _no_default_specified, so that it's clearly private, to discourage use elsewhere.

In theory this is an API-breaking change for anyone using NoDefaultSpecified in other packages. In practice I can't find any other uses within ETS, uses outside ETS seem unlikely, and I'm willing to risk the breakage.

We might also consider renaming the class to _NoDefaultSpecified, or even simply using an instance of object as the sentinel, but that can be done in a separate PR.

@kitchoi kitchoi changed the title Prevent singleton instance from name shadowing its class Fix documentation build due to class being name shadowed by a singleton Jan 4, 2021
@kitchoi
Copy link
Contributor Author

kitchoi commented Jan 4, 2021

Ah, good point about this being API-breaking.
I think I found instances of importing NoDefaultSpecified to be used in a custom Trait type definition in a private project.

@kitchoi
Copy link
Contributor Author

kitchoi commented Jan 4, 2021

I think I found instances of importing NoDefaultSpecified to be used in a custom Trait type definition in a private project.

On closing closer inspection, those codes are actually dead. But the use case remains valid to me :/

@mdickinson
Copy link
Member

mdickinson commented Jan 4, 2021

Okay. The urgent need is to get the doc build fixed, so that CI isn't blocked. For that, how about we simply change the incorrect autoclass entry for NoDefaultSpecified to autodata? We can change the code later.

@kitchoi
Copy link
Contributor Author

kitchoi commented Jan 4, 2021

I will merge this to resolve #1377 and unblock CI for PRs.
I will open an issue for revisiting how we want to expose this no-default-value singleton (and if we want to expose it at all).

@kitchoi kitchoi merged commit a68bc50 into master Jan 4, 2021
@kitchoi kitchoi deleted the fix-doc-build-error branch January 4, 2021 12:00
@mdickinson
Copy link
Member

I will merge this to resolve #1377

Please could you make the requested name change before merging?

@mdickinson
Copy link
Member

Ah, sorry; too late.

@kitchoi
Copy link
Contributor Author

kitchoi commented Jan 4, 2021

Oops, sorry. I thought the name change is related to the new issue.

@mdickinson
Copy link
Member

Oops, sorry. I thought the name change is related to the new issue.

Okay; please open the new issue and we'll discuss there. Thanks for the quick fix.

@kitchoi
Copy link
Contributor Author

kitchoi commented Jan 4, 2021

Opened #1379

mdickinson added a commit that referenced this pull request Jan 7, 2021
@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.

Documentation build is failing
2 participants