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 ETSConfig class documentation visible in the API docs #1688

Merged
merged 2 commits into from
Aug 9, 2022

Conversation

mdickinson
Copy link
Member

This PR provides an alternative to #1669. The goal is to make the ETSConfig docstrings available in the API docs, so that a search for ETSConfig in the online documentation gives useful results.

To that end, this PR:

  • Renames the ETSConfig class to ETSConfigFactory
  • Keeps the ETSConfig instance the same as before (previously, the created instance shadowed the class)
  • Updates the main docstrings of ETSConfigFactory and ETSConfig.

There shouldn't be backwards-compatibility concerns with the rename: the ETSConfig class wasn't available before (since it was shadowed by the instance), and the trick of using type(ETSConfig) to get the class will still work.

I'm deliberately not exposing ETSConfigFactory in traits.api, since in most circumstances it shouldn't be used directly.

Screenshots: before

Screenshot 2022-08-08 at 15 04 40

and after (only a portion of the docs shown):

Screenshot 2022-08-08 at 14 57 30

@mdickinson mdickinson added the component: documentation Issues related to the Sphinx documentation label Aug 8, 2022
@mdickinson mdickinson mentioned this pull request Aug 9, 2022
4 tasks
Copy link
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

I'm a little unsure if there are additional consequences of the re-naming of the class, but I think it's fine: anywhere that was using it was likely doing ETSConfig.__class__ which should still work.

@mdickinson
Copy link
Member Author

I'm a little unsure if there are additional consequences of the re-naming of the class, but I think it's fine

Yes, I think it's mostly safe on the basis that the ETSConfig type was inaccessible by name before. So anyone trying to get hold of it is presumably using .__class__ or type(), and those will still work.

A late thought on the name, before it gets set in stone: would ETSConfigType be a better name than ETSConfigFactory. Both are kinda clunky, but I think I want clunky in this case, to discourage people from trying to use them.

@corranwebster
Copy link
Contributor

A late thought on the name, before it gets set in stone: would ETSConfigType be a better name than ETSConfigFactory.

I think I prefer ETSConfigType.

@mdickinson
Copy link
Member Author

I think I prefer ETSConfigType.

Renamed. Merging when CI completes.

@mdickinson mdickinson merged commit 9e4664c into main Aug 9, 2022
@mdickinson mdickinson deleted the docs/ets-config-docs branch August 9, 2022 13:41
mdickinson added a commit that referenced this pull request Aug 9, 2022
This PR provides an alternative to #1669. The goal is to make the ETSConfig docstrings available in the API docs, so that a search for "ETSConfig" in the online documentation gives useful results.

To that end, this PR:

* Renames the ETSConfig class to ETSConfigType
* Keeps the ETSConfig instance the same as before (previously, the created instance shadowed the class)
* Updates the main docstrings of ETSConfigType and ETSConfig.

There shouldn't be backwards-compatibility concerns with the rename: the ETSConfig class wasn't available before (since it was shadowed by the instance), and the trick of using type(ETSConfig) to get the class will still work.

I'm deliberately not exposing ETSConfigType in traits.api, since in most circumstances it shouldn't be used directly

(cherry picked from commit 9e4664c)
mdickinson added a commit that referenced this pull request Aug 9, 2022
This PR provides an alternative to #1669. The goal is to make the ETSConfig docstrings available in the API docs, so that a search for "ETSConfig" in the online documentation gives useful results.

To that end, this PR:

* Renames the ETSConfig class to ETSConfigType
* Keeps the ETSConfig instance the same as before (previously, the created instance shadowed the class)
* Updates the main docstrings of ETSConfigType and ETSConfig.

There shouldn't be backwards-compatibility concerns with the rename: the ETSConfig class wasn't available before (since it was shadowed by the instance), and the trick of using type(ETSConfig) to get the class will still work.

I'm deliberately not exposing ETSConfigType in traits.api, since in most circumstances it shouldn't be used directly

(cherry picked from commit 9e4664c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: documentation Issues related to the Sphinx documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants