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

Fixed issue with logging of task name to stdout #58

Merged
merged 4 commits into from Jun 27, 2017

Conversation

4 participants
@tiagofilipe12
Member

tiagofilipe12 commented Jun 23, 2017

Previously console.log('Joining to task: ' + task.info.name) would render something like:
Joining to task: undefined. However, the task name is stored in task.info.props.name instead.

@tiagofilipe12 tiagofilipe12 self-assigned this Jun 23, 2017

@tiagofilipe12 tiagofilipe12 requested a review from thejmazz Jun 23, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Jun 23, 2017

Codecov Report

Merging #58 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #58   +/-   ##
=======================================
  Coverage   80.64%   80.64%           
=======================================
  Files          36       36           
  Lines         806      806           
  Branches       99       99           
=======================================
  Hits          650      650           
  Misses        156      156
Impacted Files Coverage Δ
lib/orchestrators/join.js 95.45% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fba1c8...ff0baae. Read the comment docs.

@@ -17,10 +17,11 @@ const join = (dispatch) => {
// TODO better checking if task/join/junction/fork
if (task.info) {
console.log('Joining to task: ' + task.info.name)
if (task.info.props) {

This comment has been minimized.

@thejmazz

thejmazz Jun 23, 2017

Member

we can consider using lodash.get for things like this (_.get(task, 'info.props') since then we wont have runtime errors like cannot get property props of undefined (if task.info does not exist)

at the same time, all properties on the task state object should be declared in the initial state (i.e. what the default value is for state in the reducer (state, action)

This comment has been minimized.

@tiagofilipe12

tiagofilipe12 Jun 23, 2017

Member

So, if I do console.log('Joining to task: ' + _.get(task, 'info.props.name')) it seems to be similar to what it was previously, but without the problem of task.info being undefined and breaking the run before it ended, right?

@@ -17,10 +17,11 @@ const join = (dispatch) => {
// TODO better checking if task/join/junction/fork
if (task.info) {
console.log('Joining to task: ' + task.info.name)
if (task.info.props) {
console.log('Joining to task: ' + task.info.props.name)

This comment has been minimized.

@thejmazz

thejmazz Jun 23, 2017

Member

is the logging function available to use here? we can do something like

logger.emit('debug', `Joining to task: ${task.info.props.name}`)

That way we can later have stdout be filtered with CLI options, like --debug to enable more verbose debugging

logger is generally passed into a function as a param (with a preset tab size), see search: logger

(we can reconsider the logger if it is overly complicated - automated indenting was tricky and perhaps not worth the complexity)

@thejmazz

This comment has been minimized.

Member

thejmazz commented Jun 23, 2017

@tiagofilipe12 ready to merge?

@thejmazz thejmazz merged commit b3e1588 into bionode:master Jun 27, 2017

4 checks passed

codecov/patch 100% of diff hit (target 80.64%)
Details
codecov/project 80.64% (+0%) compared to 7fba1c8
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk No new vulnerabilities
Details

@bmpvieira bmpvieira added the bug label Aug 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment