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

Feature/string transformer refactor #286

Merged
merged 10 commits into from
Aug 8, 2024

Conversation

Chip2916
Copy link
Collaborator

@Chip2916 Chip2916 commented Aug 7, 2024

Changes to update StringConcatenator and SeriesStrMethodTransformer tests to bring in line with new testing structure.

Issues: 278 and 279

  • As part of the StringConcatenator refactor, added a separator mixin, as the arg is also used by the OHE transformer.
  • For the SeriesStrMethodTransformer I had to overwrite one of the generic init tests as the 'columns' arg for the SeriesStrMethodTransformer can only accept a list of length 1, unique to this transformer. Could be we want to refactor the actual transformer more than I have done, which I can pick up if the reviewer thinks this is in scope.

Copy link
Contributor

@davidhopkinson26 davidhopkinson26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a case could be made for creating a test class for single column input transformers but since this is the only one it is pretty thin. Overwriting like this for one off exceptions is the intended use of the test framework.

Happy with changes, thanks!

Copy link
Collaborator Author

@Chip2916 Chip2916 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My changes approved by David, who has then resolved merge conflicts, happy with this change.

@Chip2916 Chip2916 assigned Chip2916 and unassigned Chip2916 Aug 8, 2024
@davidhopkinson26 davidhopkinson26 merged commit a21d886 into main Aug 8, 2024
12 checks passed
@Chip2916 Chip2916 deleted the feature/string_transformer_refactor branch August 8, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants