Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

refactor: avoid using lodash where possible #459

Merged
merged 4 commits into from
Mar 27, 2021

Conversation

TrySound
Copy link
Contributor

Using lodash methods does not add value where native methods and
operators can be used. In this diff I avoided lodash where possible.

Also fixed a few async tests (returned promise).

@coveralls
Copy link

coveralls commented Jul 16, 2020

Coverage Status

Coverage decreased (-0.2%) to 88.149% when pulling 67a9e5d on TrySound:avoid-lodash into 7ae82e8 on chimurai:master.

@lgtm-com
Copy link

lgtm-com bot commented Jul 16, 2020

This pull request introduces 4 alerts when merging 9b81199 into 5384dcf - view on LGTM.com

new alerts:

  • 2 for Unused loop iteration variable
  • 2 for Unused variable, import, function or class

@TrySound
Copy link
Contributor Author

Friendly ping @chimurai

@chimurai
Copy link
Owner

chimurai commented Aug 3, 2020

Hi @TrySound

Thanks for the PR.
Some effort was already started in this thread. #428

Noticed some skipped tests. What is the reason for that?

@TrySound
Copy link
Contributor Author

TrySound commented Aug 3, 2020

I wish I remember. Probably they were not stable locally even before my changes.

@TrySound
Copy link
Contributor Author

TrySound commented Aug 3, 2020

I plan to continue the effort after merging this PR.

@ghost
Copy link

ghost commented Aug 3, 2020

Congratulations 🎉. DeepCode analyzed your code in 5.507 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@TrySound
Copy link
Contributor Author

TrySound commented Aug 3, 2020

Retry DeepCode

Using lodash methods does not add value where native methods and
operators can be used. In this diff I avoided lodash where possible.
@TrySound
Copy link
Contributor Author

Hi @@chimurai is this good to go?

@chimurai
Copy link
Owner

chimurai commented Oct 11, 2020

Hi @TrySound,

Thanks for the reminder.

Question: Why were tests changed with extra return statement:
return expect(rewriter(rewriteFn)).resolves.toBe('/123/456');

Think it looks good. (Want to do some additional manual testing)

@TrySound
Copy link
Contributor Author

Some tests errored with unhandled rejection when I debugged code. Promise should be returned there. Nothing need to return explicitly with async await btw.

@TrySound
Copy link
Contributor Author

Friendly ping @chimurai

@gwellner
Copy link

gwellner commented Feb 3, 2021

Friendly ping from 2021, is this still in work ?

@chimurai chimurai merged commit 5eb8d69 into chimurai:master Mar 27, 2021
@chimurai
Copy link
Owner

Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants