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

Start documenting traits internals #958

Merged
merged 7 commits into from
Mar 25, 2020
Merged

Start documenting traits internals #958

merged 7 commits into from
Mar 25, 2020

Conversation

mdickinson
Copy link
Member

This PR represents a start on documenting Traits internals. It only covers a very small part of the story so far, but I hope it'll expand over time. We're ultimately aiming for the internals documentation to be coherent and complete. For this PR, it's obviously incomplete but I hope it's not incoherent (and if reviewers find it incoherent, it should be fixed).

It also adds the _instance_traits and _class_traits introspection methods to the API documentation, so that they can be referred to.

@mdickinson mdickinson requested a review from kitchoi March 23, 2020 11:57
Copy link
Contributor

@kitchoi kitchoi 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!
Just some nitpicks. Can be merged without a second review.

docs/source/traits_user_manual/internals.rst Outdated Show resolved Hide resolved
``HasTraits`` object ``obj``) are analogous to those for attribute retrieval.
The starting point is ``has_traits_setattro`` in the source.

- First we look for the name ``name`` in ``obj``s instance traits dictionary,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: obj's (missing an apostrophe) ?
The formatting gets a bit upset here (as shown on GitHub and with rst2html). Looks like we need an extra space between obj and "'s".
Same applies to the bullet point below.

Copy link
Member Author

Choose a reason for hiding this comment

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

I reworded to avoid the awkward possessive. See what you think.

the ``trait_object`` struct in the C source) has three relevant fields:
``getattr``, ``setattr`` and ``post_setattr``. These fields contain C function
pointers, and are invoked during attribute get and set operations, as described
below.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to check my understanding: I think the "C structure underlying each CTrait instance" is referring to the object referenced by CTrait.handler?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not the handler; I meant the trait_object C struct. I'll attempt to reword to clarify.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is, I'm referring to these three struct fields:

traits/traits/ctraits.c

Lines 217 to 219 in de2eaa2

trait_getattr getattr; /* Get trait value handler */
trait_setattr setattr; /* Set trait value handler */
trait_post_setattr post_setattr; /* Optional post 'setattr' handler */

Copy link
Member Author

Choose a reason for hiding this comment

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

I've reworded the paragraph, though I think what's really needed is a much fuller description of the mechanics. Any better?

docs/source/traits_user_manual/internals.rst Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Mar 25, 2020

Codecov Report

Merging #958 into master will increase coverage by 0.10%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #958      +/-   ##
==========================================
+ Coverage   73.05%   73.16%   +0.10%     
==========================================
  Files          51       51              
  Lines        6514     6507       -7     
  Branches     1309     1308       -1     
==========================================
+ Hits         4759     4761       +2     
+ Misses       1363     1352      -11     
- Partials      392      394       +2     
Impacted Files Coverage Δ
traits/has_traits.py 71.24% <ø> (ø)
traits/trait_handlers.py 61.11% <ø> (ø)
traits/trait_types.py 71.35% <71.42%> (-0.30%) ⬇️
traits/editor_factories.py 79.59% <0.00%> (-9.19%) ⬇️
traits/traits.py 76.92% <0.00%> (+4.04%) ⬆️
traits/etsconfig/etsconfig.py 63.46% <0.00%> (+6.41%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26548b0...3c7ab03. Read the comment docs.

@mdickinson
Copy link
Member Author

Ready for re-review, but I might find time to expand the attribute get/set stuff a bit more before this goes in, to link the getattr slot in the trait_object struct to what happens in the handler.

Copy link
Contributor

@kitchoi kitchoi left a comment

Choose a reason for hiding this comment

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

I've reworded the paragraph, though I think what's really needed is a much fuller description of the mechanics. Any better?

Yes, much clearer! Thank you!

Ready for re-review, but I might find time to expand the attribute get/set stuff a bit more before this goes in, to link the getattr slot in the trait_object struct to what happens in the handler.

I am okay with either (1) merge this first and expand it in a separate PR or (2) expand this before merging. I can also dismiss also my own review for now if it makes things clearer, just let me know.

docs/source/traits_user_manual/internals.rst Show resolved Hide resolved
@mdickinson mdickinson merged commit aab02e9 into master Mar 25, 2020
@mdickinson mdickinson deleted the doc/traits-internals branch March 25, 2020 16:04
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.

3 participants