Extra module for utility functions #62
Conversation
|
I have just realized that this way of doing this (another Cython extension) is just a dead end. Both extensions will be statically linked against ImGui and this will break the whole integration of C++ assert statements with Python exceptions. Also this may break in other unexpected ways. Right now I so see only following solutions to this isse:
|
| core.vertex_buffer_vertex_uv_offset = _core_to_extra_deprecated(extra.vertex_buffer_vertex_uv_offset) # noqa | ||
| core.vertex_buffer_vertex_col_offset = _core_to_extra_deprecated(extra.vertex_buffer_vertex_col_offset) # noqa | ||
| core.vertex_buffer_vertex_size = _core_to_extra_deprecated(extra.vertex_buffer_vertex_size) # noqa | ||
| core.index_buffer_index_size = _core_to_extra_deprecated(extra.index_buffer_index_size) # noqa |
There was a problem hiding this comment.
Random question, what does the # noqa comment mean?
There was a problem hiding this comment.
This common mark for linters like pep8 (and many IDEs) to exclude current line from style checking. This line is longer than 79 columns but splitting into multiple lines would IMHO reduce readability.
Such things are already my habit. Now I see that project doesn't have linting in build pipeline. This is oversight. I will add another stage to Travis CI config so all # noqa will have a bit more sense.
| vertex_buffer_vertex_uv_offset = core._py_vertex_buffer_vertex_uv_offset | ||
| vertex_buffer_vertex_col_offset = core._py_vertex_buffer_vertex_col_offset | ||
| vertex_buffer_vertex_size = core._py_vertex_buffer_vertex_size | ||
| index_buffer_index_size = core._py_index_buffer_index_size |
There was a problem hiding this comment.
I like this, using the ._py_ prefix for the actual code, but exposing the function at the top level and without the prefix. 👍
| but are useful in Python application. | ||
|
|
||
| """ | ||
| from . import core |
There was a problem hiding this comment.
Can this just be import core ?
There was a problem hiding this comment.
import core will fail on Python 3 as it no longer supports implicit relative imports.
setup.py
Outdated
| @@ -81,7 +81,21 @@ def get_version(version_tuple): | |||
| general_macros = [] | |||
|
|
|||
|
|
|||
| extension_sources = ["imgui/core" + ('.pyx' if USE_CYTHON else '.cpp')] | |||
| def extension_sources(path): | |||
| sources = [path + ('.pyx' if USE_CYTHON else '.cpp')] | |||
There was a problem hiding this comment.
Super nitpicky, but it's a little safer to do string interpolation. "{0}{1}".format(path, '.pyx' if USE_CYTHON else '.cpp'
There was a problem hiding this comment.
Good point, it will look better.
|
LGTM, just a couple of small things. The build is failing, but it's not entirely clear why? Pythonhosted was giving 404s? You might give it a retry. |
|
These pip failures are strange indeed. I have restarted Travis build immediately but apparently forgot about Appveyor. |
|
Apparently this was just a transient issue. |
|
👍 |
|
Released as |
Refers to #33.
This creates a new extension named
imgui.extrathat contains all additional utility functions that are not part of core C++ API. It also leaves aliases with deprecation warnings for old function names inimgui.coreextension that will re removed in 1.0.0 version.This unfortunately increases the project complexity a bit but it is possible that net complexity will decrease over time as we start to add new helpers (maybe even custom python-specific widgets!) and after we remove the deprecated aliases.
@pconerly would you like take a look and review it?