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

Add macro to create register functions and move laplacian to single file #10

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

luisfpereira
Copy link
Owner

@luisfpereira luisfpereira commented Dec 17, 2024

This PR adds a macro to automatically create register_ functions, and moves everything that was inside laplacian folder to a single file (previous design was due to less robust registry; this way is simpler).

The register macro works as follow: given a Registry (e.g. LaplacianFinderRegistry) it creates a function register_ by removing Registry from the name and transforming it in snake case (e.g. register_laplacian_finder).
(This is a simple thing that may potentially make the code slightly harder to understand - you tell me @GiLonga and @gviga -, but it is annoying to create this functions every time).

@luisfpereira luisfpereira changed the title Registry Add macro to create register functions and move laplacian to single file Dec 17, 2024
@luisfpereira luisfpereira merged commit f354edb into main Dec 17, 2024
1 check failed
@luisfpereira luisfpereira deleted the registry branch December 17, 2024 02:38
@GiLonga
Copy link
Collaborator

GiLonga commented Dec 17, 2024

I agree with you. Just yesterday, I was struggling with those lines of code myself. This new approach is more elegant but a bit tricky to understand at first glance. I think a more detailed docstring is necessary, something like:

Automatically create `register_` functions for each class registry in this module. 
These registers will be useful subsequently for operations in the Mixins classes. 
Example:
class LaplacianFinderRegistry(MeshWhichRegistry):
    MAP = {}
LaplacianFinderRegistry.register  -> this operation is now automatically performed by this function.

An example would help to clarify it.

@luisfpereira
Copy link
Owner Author

Spot on @GiLonga. I've updated the docstring (b61af64). Hope it makes it more clear. I think the example should be given within the Registry docstrings though. I'll update those soon for clarity.

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.

2 participants