refactored lodash out #31
Conversation
Ore4444
commented
Dec 1, 2019
•
Ore4444
commented
- left only a few places that use specialized functions of lodash. replaced all the rest with native functions
- removed util.ts in favor of utils/ folder where each utility function is in a separate file (salvaged a bit from milesj 2.0 branch)
- removed lodash property from vorpal module
- applied bug fix to variadic arguments from milesj 2.0 branch (buildCommandArgs.ts)
There was a problem hiding this comment.
My two cents:
- It's one thing to reduce usage of lodash for vanilla usage, it's totaly another one to totally ditch it.
(not to say to replace it with handmade untested functions) - I'm not a huge fan of having a file per utils, where a lot of file amount the 16 are 3 liners, and with a plumbing 17 lines as index! 😷
|
|
Codecov Report
@@ Coverage Diff @@
## develop #31 +/- ##
===========================================
+ Coverage 62.26% 62.35% +0.09%
===========================================
Files 14 17 +3
Lines 1582 1578 -4
Branches 390 395 +5
===========================================
- Hits 985 984 -1
+ Misses 591 586 -5
- Partials 6 8 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
LGTM.
We could consider replacing these functions with native ones as well:
isFunction-->typeof a === 'function'
https://stackoverflow.com/questions/17108122/isfunctiona-vs-typeof-a-function-javascript/17108198#17108198isUndefined-->typeof a === 'undefined'
https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore#_isundefinedisString-->typeof a.valueOf() === 'string'- https://stackoverflow.com/a/58892465/938297uniq-->[...new Set(a)]- https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore#_uniq
Also, I'd be happy not to rely on lodash for trivial things like noop.
src/utils/index.ts
Outdated
| @@ -0,0 +1,8 @@ | |||
| export {buildCommandArgs} from './buildCommandArgs'; | |||
There was a problem hiding this comment.
You can prevent some boilerplate with
export * from './buildCommandArgs';
// etc.There was a problem hiding this comment.
I actually prefer making the export explicit, in case we want to add private helper functions inside the util files, or common helpers to all utils which we don't want to mistakingly export by default. WDYT?
There was a problem hiding this comment.
Private helper functions wouldn't be exported so they will not pollute the index module, and I guess common helpers would go in their own files? 😉
There was a problem hiding this comment.
Many things are still to be tuned IMO,
Taking into account your wish to smooth things up (cf the slack #general)
I won't formally Request Changes So that you can get it merge if ever you find more people thinks that can be incorporated as is
src/utils/parseCommand.ts
Outdated
| @@ -0,0 +1,105 @@ | |||
| import {ICommand, IMatchParts, InputCommand} from '../types/types'; | |||
There was a problem hiding this comment.
would go perfectly with matchCommand as the two imports line there hint
There was a problem hiding this comment.
Far better! :)
Good work @Ore4444 !
Almost mergeable, tell me what you think about these last comments.
(main one being in src/utils/index.ts: https://github.com/vorpaljs-reforged/vorpal/pull/31/files/f1692e44bc3a6e8523fbadfa0b20948f32c0c34b#diff-2cfb1554a9cbe2bf7981031a26f82e4e)
| commandParts.args = (utils.buildCommandArgs( | ||
| commandParts.args, | ||
| commandParts.command | ||
| ) as unknown) as string; |
There was a problem hiding this comment.
feels a bit weird that double cast. Seems a bad smell to me
(or a sign I indeed need raising my typing level :))
There was a problem hiding this comment.
Good for me!! :)
Thanks for the work @Ore4444!
(and sorry for this review that could have come quicker if I wasn't that much into digestion mode 😁)
