-
Notifications
You must be signed in to change notification settings - Fork 170
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
Extract utility functions #150
Conversation
85a350c
to
34a12e1
Compare
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.
Overall LGTM!
if ( | ||
workspace.services && | ||
typeof workspace.services === 'string' && | ||
workspace.services.length > 0 |
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 is no longer necessary?
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.
thanks - must have slipped my sight. will fix in new commit
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.
that's already handled in the convertJSON
method
if ( | ||
workspace.data && | ||
typeof workspace.data === 'string' && | ||
workspace.data.length > 0 |
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.
Same here (?)
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.
that's already handled in the convertJSON
method
if ( | ||
service.settings && | ||
typeof service.settings === 'string' && | ||
service.settings.length > 0 |
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.
And here (?)
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.
that's already handled in the convertJSON
method
34a12e1
to
b08cdb6
Compare
Pre-flight Checklist
Description of Change
Extracted common utility functions related to JSON parsing
Will wait for #149 to be merged, and then put this up for review.
Motivation and Context
Remove code duplication
Screenshots
Checklist
npm run prepare-code
)npm test
passesRelease Notes