Fixes #43: Debug kwargs removal #44
Conversation
Fixes issue where 'base.BaseTransformer' was accepting kwargs even when its parent classes did not expect any. This was progarting as a bug where any unexpected/wrong argument passed to child classes did not raise any error.
|
Hi,
|
|
Hi Cristian, Thank you very much for the contribution! Yes, please make the PR into develop rather than main. I have made another PR into develop to fix the black issue separately #45. On versioning - we will probably put this in a small release with #45 which will be 0.3.3. Version 0.4 is however likely to be ready in December with lots of new transformers so we may roll in these changes into a single release. For the purposes of this PR please set version to 0.3.3 and update the change log with this too for now. You went a bit further than I expected by removing kwargs from all transformers and changing to explicitly pass copy and verbose. For the issue in #43 need it would be sufficient to remove **kwargs from BaseTransformer only and continue to have those two arguments passed using **kwargs from its children. If unexpected args were passed they would still be picked up by BaseTransformer and raise an error. Apologies if this wan't clear in my comments on the issue. This structure was intended to help with maintainability since it allows us to add new options at the BaseTransformer level without needing to change every inheriting transformer and their tests. It also reduces clutter. This is however at a slight trade off with ease of use for these two features, and the code is possibly harder to understand. I can see you have put a lot of work into the changes so I am keen to hear your thoughts on removing **kwargs just from the base transformer or from all children too before we decide which direction to take here. |
| @@ -367,12 +369,16 @@ def __init__( | |||
| new_column_name, | |||
| pd_method_name, | |||
| columns, | |||
| pd_method_kwargs={}, | |||
| pd_method_kwargs=None, | |||
There was a problem hiding this comment.
Can you explain your reasoning for changing this?
|
Hi, thanks for your contribution here! It has helped identify a number of tests which were not functioning properly. I have taken the key components of the fix into #56 and merged into develop for the next release. Closing this for now. |
Fixes issue #43 where 'base.BaseTransformer' was accepting kwargs even when its parent classes did not expect any. This was propagating as a bug where any unexpected/wrong argument passed to child classes did not raise any error.
There were some unit tests that where failing because of old used arguments that went silently and one that even had a wrong argument passed to.
Some notes on the PR:
Overall it was fun to solve, but a bit too long. A recommendation would be to remove the possibility to have the arguments 'columns', 'copy' and 'verbose' from the 'BaseTransformer', but that is just taste/design choice and not a bug or issue.