Open JMorado opened 1 year ago
Refactoring looks like a good idea to me. Would we break it down into functions, then call these functions from the init method, as is done with _remove_duplicate_points()?
@rdPatmore Yes, exactly. Then we can also create unit tests for these private methods to ensure that initialization goes as expected.
@JMorado Ok, I'll put myself down as an assignee, with a view to do some refactoring.
@JMorado I have created a branch for this issue and completed some initial refactoring. We may wish to make further modifications to this new structure. The functions could also do with better documentation (i.e. more detailed docstrings).
@rdPatmore, I have now finished the refactoring process, added appropriate docstrings, and included type hints where necessary.
I think we are now ready to start writing unit tests for every method. This will also tell us if further modifications to the class are needed.
@rdPatmore, we've got a few functional/unit tests up and running. If you're interested, we could catch up and chat about our next steps. Let me know!
@JMorado great! I've had a quick look at the code. Much improved. Happy to chat at somepoint over the next week.
@rdPatmore, I have completed this task. I will create a PR for this issue.
@jdha, regarding the docstrings, see function_with_pep484_type_annotations example for an example close to what we are doing. In my opinion, replicating the types in the docstring might be unnecessary since they are already specified through the type annotations. Currently, we are not duplicating the return type in the docstring in order to adhere to the NumPy docstring style. However, we could consider adding this duplication at a later time.
As a starting point for having unit tests in PyBDY, it could be interesting to focus on the
Boundary
class defined innemo_bdy_gen_c.py
. From discussions with @jdha, we concluded that this class is rather self-contained, which makes it ideal to start with.However, I believe that before writing unit tests for the methods of this class, the class itself should be slightly refactored. The
__init__
constructor method is currently performing too many operations. Most of the code in this constructor should be moved to private methods. Additionally, I plan to document the whole module, which could serve as an example for the future.Are there any objections to refactoring this class? Does anyone have other ideas on how to proceed?
Many thanks.