Skip to content

Conversation

@johmedr
Copy link
Collaborator

@johmedr johmedr commented Mar 20, 2025

This follows from discussions (#22) about debloating the wrapper.py file.

Main changes

The file is now split it several files:

  • spm.__wrapper__.core contains most of the new type system's mechanics
  • spm.__wrapper__.utils contains private helper functions
  • spm.__wrapper__.helpers contains what was previously in spm.cheats
  • The main public types have each their own files, e.g., spm/__wrapper__/Cell.py
  • The global matlab variable is removed, the matlab module can be obtained by matlab = _import_matlab() (@balbasty assuming this temporary import mechanism is still necessary)

Limitations

  • The current design necessitates a lot of circular imports, that are made local and flagged with a FIXME when possible. Ultimately, classes in core should not depend on external classes. Moreover, the type hierarchy should be respected. One of the reason for circular imports is that we use the main MatlabType as a factory (when using from_any). We could have an external MatlabTypeFactory handling the role of instantiating new objects.

@johmedr
Copy link
Collaborator Author

johmedr commented Mar 20, 2025

Not sure about why the tests are failing on the server, they all pass locally...

@johmedr johmedr requested a review from balbasty March 20, 2025 03:14
@balbasty
Copy link
Contributor

balbasty commented Mar 20, 2025

It seems because in __wrapper__.__init__ you use snake_case for module names whereas in reality you use CamelCase:

from .runtime import Runtime
from .matlab_class import MatlabClass
from .matlab_function import MatlabFunction
from .cell import Cell
from .struct import Struct
from .array import Array
from .sparse_array import SparseArray

That said, I do prefer snake_case for module names (from .cell import Cell instead of from .Cell import Cell).

It all looks great otherwise!

@balbasty
Copy link
Contributor

balbasty commented Mar 20, 2025

For circular imports, we could also monkey patch MatlabType in __wrapper__.__init__, i.e., once everything is imported, assign the factory function:

from .runtime import Runtime
from .matlab_class import MatlabClass
from .matlab_function import MatlabFunction
from .cell import Cell
from .struct import Struct
from .array import Array
from .sparse_array import SparseArray

from .core import MatlabType
from .factory import MatlabTypeFactory
MatlabType.from_any = MatlabTypeFactory.from_any

That's only if we want to eventually expose all core types in a potential mpython or mpython-types package. In SPM, simply moving the factory in MatlabTypeFactory would suffice.

@johmedr
Copy link
Collaborator Author

johmedr commented Mar 20, 2025

Thank you for your comment, it put me on the way. It turns out that it was working on my side because the files had been renamed to snake_case, but apparently Git does not consider case change as a change, so the new name did not propagate, giving this issue. Using git mv did the trick. That was an interesting one!

@johmedr
Copy link
Collaborator Author

johmedr commented Mar 20, 2025

Merging as PR approved and tests passing.

@johmedr johmedr merged commit 7e437de into main Mar 20, 2025
32 checks passed
@johmedr johmedr deleted the dev-separate-wrapper branch March 20, 2025 12:31
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