-
Notifications
You must be signed in to change notification settings - Fork 58
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
Add ObjectUtil helper class #9
Conversation
cf4b04f
to
07f5886
Compare
07f5886
to
c897a6d
Compare
Basically 👍 However, |
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.
Just two minors but i think they should be addresed.
As a new feature is introduced a new minor release would be good (version in package.json + tag).
src/Util/ObjectUtil.js
Outdated
var queryMatch; | ||
|
||
if (!isString(queryKey)) { | ||
// TODO Replace with new logger after logger implementation |
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.
As we got a Logger now please address this util.
src/Util/ObjectUtil.js
Outdated
return false; | ||
} | ||
if (!isObject(queryObject)) { | ||
// TODO Replace with new logger after logger implementation |
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.
As we got a Logger now please address this util.
@KaiVolland I have addressed your two comments, would you have another look?
OI think before refactoring we should add it as is. If you want me to do it, I can of course go ahead and rewrite this. |
…il-0.1.1 Update @terrestris/base-util to the latest version 🚀
This PR adds an
ObjectUtil
helper class from a client project. It is ported with the minimum needed changes, and with additional tests so that coveralls is happy.Please review.