Skip to content

Conversation

jozic
Copy link
Collaborator

@jozic jozic commented Dec 29, 2023

No description provided.

@jozic jozic marked this pull request as ready for review December 29, 2023 05:44
@jozic
Copy link
Collaborator Author

jozic commented Dec 29, 2023

@ckipp01 if you have a couple minutes, please check it out

Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Nice clean up!

Copy link
Member

Choose a reason for hiding this comment

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

This is all much nicer to work with. I know you have it covered with a bunch of itest, nice job adding those, but I also always find it worth the time to unit test classes like these as you often get way more coverage that way, and it's easy to miss something. But it's not a blocker.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the review!
I was on the fence myself with adding unit tests or not as there were none here , but after your comment I decided that it won't hurt to have those as well :)

@jozic jozic merged commit 051311d into master Dec 30, 2023
@jozic jozic deleted the skip-tests branch December 30, 2023 23:52
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