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.

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

Simplifications #3

Merged
merged 6 commits into from
May 29, 2018
Merged

Simplifications #3

merged 6 commits into from
May 29, 2018

Conversation

nerac
Copy link
Contributor

@nerac nerac commented May 9, 2018

No description provided.

index.js Outdated
let obj = parseArgs(args)
log[level](obj.message, obj);
}
const logger = Object.assign({},...Object.keys(log.levels).map(level => ({[level]: logByLevel(level)})));
Copy link

@rubanour rubanour May 9, 2018

Choose a reason for hiding this comment

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

I think we can use reduce insteal of Object.assign and map. something like
Object.keys(log.levels).reduce((acc, level) => acc[level] = logByLevel(level), {})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which one you like more?

const logger = Object.assign({},...Object.keys(log.levels).map(level => ({[level]: logByLevel(level)})));

const logger = Object.keys(log.levels).reduce((acc, val) => { acc[val] = logByLevel(val); return acc; },{}); 

I have the feeling that the second one is much clear.

Copy link

Choose a reason for hiding this comment

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

same here, the second one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ;)

index.js Outdated
message:objToLog.message.concat(util.inspect(arg)),
}) || isObject(arg) && Object.assign({}, objToLog, arg) || Object.assign({},objToLog,{
message:objToLog.message.concat(util.inspect(arg)),
});
Copy link

Choose a reason for hiding this comment

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

I think using many shorthand expressions here makes it a bit harder to read.
Also, I think it might be better if we change parseArgs to return { message, context } and don't initialize message inside objtoLog in the first place. something like in here f164f8d#diff-168726dbe96b3ce427e7fedce31bb0bcR29

@nerac
Copy link
Contributor Author

nerac commented May 9, 2018

@rubanour Applied ;)

index.js Outdated
};
const parseArgs = (args) => {
const { messages, context } = args.reduce((acc, arg) => {
return !arg && acc || addError(arg,acc) || addObject(arg,acc) || Object.assign({},acc,{messages:addMessage(arg,acc)});
Copy link

Choose a reason for hiding this comment

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

arg instead of !arg .. right?
we can remove the brackets and the return as well :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, should be !arg as if there is no argument we return the acc and only if we have an argument we execute the assignation chain.

Respect brackets...yes 😅 I'll do it.

@nerac nerac force-pushed the fp branch 2 times, most recently from 669477c to 0b3cde3 Compare May 9, 2018 13:43
@nerac nerac merged commit d5e947e into master May 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants