Skip to content

Refactor index.js & add more tests - #223

Open
mfeniseycopes wants to merge 5 commits into
LogRocket:masterfrom
mfeniseycopes:refactor
Open

mfeniseycopes wants to merge 5 commits into
LogRocket:masterfrom
mfeniseycopes:refactor

Conversation

@mfeniseycopes

Copy link
Copy Markdown

Refactors index.js to utilize more helper methods and make code more semantic. No changes in logic or naming conventions, just shifting things around. Also, moves early returns to the top of createLogger to prevent unnecessary assignments and variable creation.

Also, adds more specs to index.spec.js (still needs more though) and moves helper tests to helpers.spec.js.

Let me know what you think!

@codecov-io

codecov-io commented Apr 16, 2017 •

Copy link
Copy Markdown

Codecov Report

Merging #223 into master will decrease coverage by 14.21%.
The diff coverage is 89.28%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #223       +/-   ##
===========================================
- Coverage   83.56%   69.34%   -14.22%     
===========================================
  Files           5        5               
  Lines         146      137        -9     
===========================================
- Hits          122       95       -27     
- Misses         24       42       +18
Impacted Files Coverage Δ
src/index.js 89.74% <89.28%> (+2.9%) ⬆️
src/diff.js 8% <0%> (-92%) ⬇️
src/core.js 78.68% <0%> (+2.63%) ⬆️

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 07cb031...ace19b8. Read the comment docs.

@imevro

imevro commented Apr 19, 2017

Copy link
Copy Markdown
Collaborator

Sorry for delay, will review on this week.

@imevro

imevro commented Apr 19, 2017

Copy link
Copy Markdown
Collaborator

But it looks pretty good!

Comment thread src/index.js Outdated
logErrors,
diffPredicate,
} = loggerOptions;
function noLogger(options) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

hasLogger maybe?

@imevro

imevro commented May 17, 2017

Copy link
Copy Markdown
Collaborator

Can you please fix merge conflicts?

@mfeniseycopes

mfeniseycopes commented May 19, 2017 •

Copy link
Copy Markdown
Author

Renamed #noLogger to #hasLogger and rebased to resolve merge conflicts.

@mfeniseycopes

Copy link
Copy Markdown
Author

@evgenyrodionov Have you had a chance to look this over?

@imevro

imevro commented Jun 30, 2017 •

Copy link
Copy Markdown
Collaborator

@mfeniseycopes yes, after #234. Sorry for a delay, too busy now.

@vish288 vish288 mentioned this pull request Sep 8, 2017

This branch has not been deployed

No deployments
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.

3 participants