Expose module constants in traits.ctraits #688
Conversation
modified: traits/ctraits.c modified: traits/tests/test_ctraits.py
|
How does this interact with #680? (Disclaimer: I haven't looked at either PR yet.) |
Codecov Report
@@ Coverage Diff @@
## master #688 +/- ##
==========================================
+ Coverage 69.43% 69.65% +0.22%
==========================================
Files 40 40
Lines 6282 6282
Branches 1295 1295
==========================================
+ Hits 4362 4376 +14
+ Misses 1521 1504 -17
- Partials 399 402 +3
Continue to review full report at Codecov.
|
This PR doesn't interact with #680 at all because I haven't updated the rest of traits to use these newly available constants from |
There was a problem hiding this comment.
LOKTM, but needs merge conflicts resolved and error handling added.
There are a couple of promised follow-on PRs; this PR won't really make much sense without them. @rahulporuri How would you feel about expanding this PR to include the uses of the constants. Otherwise, we end up with duplication in master, and there's a risk that we don't get around to the later PRs.
modified: traits/ctraits.c modified: traits/tests/test_ctraits.py
…-module-constants-ctraits
modified: traits/constants.py modified: traits/tests/test_ctraits.py
modified: traits/ctraits.c
modified: traits/tests/test_ctraits.py
modified: traits/ctraits.c
| @@ -10,6 +10,19 @@ | |||
|
|
|||
| from enum import IntEnum | |||
|
|
|||
| from traits.ctraits import ( | |||
There was a problem hiding this comment.
This introduces a circular import: traits.trait_base imports from traits.constants, which imports from traits.ctraits, which imports from traits.trait_base. The solution may be to lift CoercableTypes to trait_handlers, which is the only place that it's used.
There was a problem hiding this comment.
Or perhaps put No, moving Undefined and Uninitialized somewhere other than in trait_base?CoercableTypes is better.
| @@ -4908,6 +4908,46 @@ PyMODINIT_FUNC PyInit_ctraits(void) { | |||
| (PyObject *) &trait_type ) < 0 ) | |||
| return NULL; | |||
|
|
|||
| /* Default value type constants */ | |||
| if ( PyModule_AddIntConstant( | |||
There was a problem hiding this comment.
I'm not especially keen on the assign-and-test style, in general. I think it would be more readable to separate the assignment from the error check. Also, let's follow PEP 7 in new code, and always use braces, even for single-statement blocks.
err = PyModule_AddIntConstant(module, "_CONSTANT_DEFAULT_VALUE", CONSTANT_DEFAULT_VALUE);
if (err) {
return NULL;
}
|
@rahulporuri Any idea when you might be able to get to the updates on this PR? |
I don't think I will be able to work on this PR anytime soon (~ 2 weeks) |
|
Okay. I'll try to pick this up at some point. |
|
Deferring this; it's not essential for 6.0, and there's no user-facing behaviour change. |
|
|
||
| #: A new instance of TraitListObject constructed using the default_value list | ||
| #: is the default value. | ||
| trait_list_object = 5 | ||
| trait_list_object = _TRAIT_LIST_COPY_DEFAULT_VALUE |
There was a problem hiding this comment.
Naming nitpick: we probably want _TRAIT_LIST_OBJECT_DEFAULT_VALUE (and similarly for trait_dict_object)
|
Closing in favour of #935. |
fixes #686
This PR exposes all of the module-level constants used in the
traits.ctraitsmodule. Exposing these constants will remove the need to duplicate the constants elsewhere in the traits codebase (This will be done in a separate PR).Note that there are a new number of constants that need to be defined and exposed in
traits.ctraits- which will also be done in a separate PR.