-
Notifications
You must be signed in to change notification settings - Fork 27
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
Introduce some utils #431
Introduce some utils #431
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks solid, thanks for this!
Please merge after addressing the typo/doc coments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs tests. Changes themself look good to me.
@marcjansen i added the most basic tests in e9a6604, but you're right. we should add "real" tests in follow-up PRs |
Thx for the reviews, i'll merge now |
Together with @weskamm i migrated some utils from projects to BasiGX.
Major changes are only in 9c4905e so it makes sense to have a sharper look here.
All other utils are new and did not lead to changes in existing classes.