Skip to content
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.

Sign up for GitHub

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

Jump to bottom

stringify-error: allows stringify on error objects #21

Merged
merged 2 commits into from
Jul 28, 2017
Merged

stringify-error: allows stringify on error objects #21

merged 2 commits into from
Jul 28, 2017

Conversation

vitorabner
Copy link
Contributor

closes #19

@vitorabner vitorabner changed the title Fix/stringify error object stringify-error: allows stringify on error objects Jul 17, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 93.333% when pulling f493016 on fix/stringify-error-object into 39298be on master.

@vitorabner vitorabner self-assigned this Jul 17, 2017
mccraveiro
mccraveiro suggested changes Jul 17, 2017
View reviewed changes
src/utils.js Outdated
const stringify = msg => {
if (R.is(String, msg)) return msg
return JSON.stringify(msg)
const buildErrorObject = ({ message, stack }) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use

const buildErrorObject = pick(['message', 'stack'])

src/utils.js Outdated

const stringify = log => {
if (R.is(String, log)) return log
if (log.message instanceof Error) log.message = buildErrorObject(log.message)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use is(Error, log.message)

src/utils.js Outdated
}

const pickProperties = (message, propsToLog) => (
pokeprop(propsToLog, message)
const pickProperties = (log, propsToLog) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use R.flip here

const pickProperties = flip(pokeprop)

src/utils.js Outdated
)

const stringify = log => {
if (R.is(String, log)) return log
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove R. to improve readability by using destructuring on require

const { is } = require('ramda')

mccraveiro
mccraveiro reviewed Jul 17, 2017
View reviewed changes
src/utils.js Outdated
const stringify = log => {
if (R.is(String, log)) return log
if (log.message instanceof Error) log.message = buildErrorObject(log.message)
return JSON.stringify(log)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(optional) Go full ramda!

const stringifyMessage = ifElse(
  is(Error),
  pick(['message', 'stack']),
  identity
)

const stringify = ifElse(
  is(String),
  identity,
  pipe(
    over(lensProp('message'), stringifyMessage),
    JSON.stringify
  )
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dá para simplificar com when e unless :)

const stringifyMessage = when(
  is(Error),
  pick(['message', 'stack'])
)

const stringify = unless(
  is(String),
  pipe(
    over(lensProp('message'), stringifyMessage),
    JSON.stringify
  )
)

@vitorabner vitorabner force-pushed the fix/stringify-error-object branch 2 times, most recently from 2e6c2c3 to bb2f5db Compare July 21, 2017 05:43
@coveralls
Copy link

Coverage Status

Coverage increased (+2.8%) to 95.349% when pulling bb2f5db on fix/stringify-error-object into 39298be on master.

mccraveiro
mccraveiro reviewed Jul 21, 2017
View reviewed changes
Copy link
Contributor

@mccraveiro mccraveiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nao acha mais limpo importar o ramda desestruturado?

@vitorabner
Copy link
Contributor Author

Sim, vou fazer isso em seguida em um PR de refactoring. Tenho que tirar os parênteses dos retornos implícitos também.

mccraveiro
mccraveiro previously approved these changes Jul 21, 2017
View reviewed changes
@vitorabner vitorabner added the bug label Jul 23, 2017
@vitorabner vitorabner force-pushed the fix/stringify-error-object branch 3 times, most recently from f99e157 to 99c97e6 Compare July 28, 2017 02:54
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 99c97e6 on fix/stringify-error-object into ** on master**.

@vitorabner
stringify-error: allows stringify on error objects
beb3696 
The native method JSON.stringify doesn't stringify properties from
prototypes. For this reason we need to check if a object is a instance
of an error, clone it to flat its props to finally use stringify.
@vitorabner
stringify-error: improve stringify unit tests
01aa989 
Add tests to check if strinfigy can handle error object
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 01aa989 on fix/stringify-error-object into ** on master**.

@edrzmr edrzmr merged commit 4935280 into master Jul 28, 2017
@edrzmr edrzmr deleted the fix/stringify-error-object branch July 28, 2017 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edrzmr edrzmr edrzmr approved these changes

@mccraveiro mccraveiro Awaiting requested review from mccraveiro

Assignees

@vitorabner vitorabner

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error object isn't logging
4 participants
@vitorabner @coveralls @edrzmr @mccraveiro