HasRequiredTraits class #419
Conversation
|
This is great; thank you! What would be really useful would be to also have documentation on this. I think the right place for that would be in https://github.com/enthought/traits/blob/master/docs/source/traits_user_manual/advanced.rst Would you be up to writing some documentation for this? |
|
Definitely! For some reason when documentation got mentioned before I somehow parsed it as docstring. Completely forgetting about the actual documentation we write! |
|
Ok, after some fighting with sphinx/restructured text this should be ready for re-review. |
| All required traits have to be provided as arguments on creating a new | ||
| instance:: | ||
|
|
||
| >>> new_instance = RequiredTest(required_trait=13.0) |
There was a problem hiding this comment.
Please can you fix up the indentation to be consistent? (This code block is indented 3 spaces; the one above uses 4 spaces, and the ones below are 2 spaces. Pick one!)
There was a problem hiding this comment.
FWIW, most of the rest of this document seems to use 4 spaces, so I'd go with that.
There was a problem hiding this comment.
Looks like I need to sort out my tabs/spaces configs in atom!
There was a problem hiding this comment.
Yeah, reST is weird, though. It encourages 3-space indents, so you tend to find a mix of 3-space and 4-space indents actual reST documentation.
| ''''''''''''''''' | ||
|
|
||
| This class builds on the functionality of HasStrictTraits and ensures | ||
| that any object attribute with `required=True` in its metadata must be passed |
There was a problem hiding this comment.
reST nit: use double backticks for code snippets. (Single backticks use the Sphinx default role, which isn't what you want here.)
| HasRequiredTraits | ||
| ''''''''''''''''' | ||
|
|
||
| This class builds on the functionality of HasStrictTraits and ensures |
There was a problem hiding this comment.
It might make sense to replace the somewhat vague "builds on" with something more precise. For example "This class inherits from HasStrictTraits [...]". Or even: "This subclass of HasStrictTraits ensures that ...".
|
This is looking good; I've left a couple of nitpick-level comments. |
There was a problem hiding this comment.
LGTM.
@corranwebster: want to do a final review before I merge?
From the docstring: This class builds on the functionality of HasStrictTraits and ensures that any object attribute with the metadata required=True must be passed as an argument on object initialization.
This came up as there were situations where a class had a few 'key' traits and the others were derived from them. It made sense to try and provide a more formal implementation of this pattern!
Things that I'm not sure about: