-
Notifications
You must be signed in to change notification settings - Fork 21
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
Fix issue 87 #88
Fix issue 87 #88
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.
Thanks, before creating such general rule I'd like to know what exactly are these erroneous cases that would be replaced, would you create a list of these and post it here?
Please don't change package-lock.json
unless there is a problem with the dependencies, in that case would you revert this change?
Sorry about modifying package-lock.json. I have reverted it. I updated code to log erroroneous objects to stderr. Only 1 object found:
|
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, this fixes #87, and is related to #31.
The logging in readCrops
is okay for now as this function is used only for the migration process and tests (for tests it can be disabled with npm run test 2>/dev/null
), but will probably be modified during #89 when more abstract modules are created for managing the Crop
and PracticalplantsCrop
types.
Thanks for feedback. Would it be better to log to a file? Open to
suggestions.
…On Sun, Aug 25, 2019 at 11:45 AM petteripitkanen ***@***.***> wrote:
***@***.**** approved this pull request.
Thanks, this fixes #87
<#87>, and is related
to #31 <#31>.
The logging in readCrops is okay for now as this function is used only
for the migration process and tests (for tests it can be disabled with npm
run test 2>/dev/null), but will probably be modified during #89
<#89> when more
abstract modules are created for managing the Crop and PracticalplantsCrop
types.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#88?email_source=notifications&email_token=ALAQO35UVVBORT43KX4OYILQGLHK3A5CNFSM4IPEWWXKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCCTIJGY#pullrequestreview-279348379>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALAQO35A77Z2SDLMLA7NIYTQGLHK3ANCNFSM4IPEWWXA>
.
|
It could be clearer to log in the tests, but only if |
Ah I see. Good idea.
…On Sun, Aug 25, 2019 at 12:55 PM petteripitkanen ***@***.***> wrote:
It could be clearer to log in the tests, but only if ENABLE_LOGGING is
true. Then this constant would be committed as false and used only
locally when needed.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#88?email_source=notifications&email_token=ALAQO37MC6FGWWQYCRNHNBTQGLPUVA5CNFSM4IPEWWXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5C2RLY#issuecomment-524658863>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALAQO36MCN4EUHYYROPR62LQGLPUVANCNFSM4IPEWWXA>
.
|
Issue 87: Fix practicalplants name properties to start with upper case letter