Use lodash directly and add include and exclude option
#13
Conversation
XhmikosR
changed the title
exclude optioninclude and exclude option
|
Added |
|
|
||
| replace({ | ||
| regex: data.pattern | ||
| , replacement: data.replacement | ||
| , paths: _.isArray(data.path) ? data.path : [data.path] | ||
| , include: _.isArray(data.include) ? _(data.include).toString() : data.include |
There was a problem hiding this comment.
Rather than passing an empty string for include and exclude when the user doesn't specify them, how about we pass in undefined?
There was a problem hiding this comment.
I'm not sure if that's any better... But it's your call :)
There was a problem hiding this comment.
BTW,
- jshint fails due to line length. Maybe I should move the checks in lines 26-27 instead.
includeandexcludeare mutually exclusive. Not sure if we should add a check for that but "replace" itself doesn't show anything in that case.
There was a problem hiding this comment.
I'm not sure if that's any better... But it's your call :)
You're right, I took a look at the replace source code and either way should work.
jshint fails due to line length. Maybe I should move the checks in lines 26-27 instead.
Or you can remove that jshint option. Either way works for me.
include and exclude are mutually exclusive. Not sure if we should add a check for that but "replace" itself doesn't show anything in that case.
I'll leave that up to you. I don't have a preference.
|
@jharding: I removed I think it's ready to merge. If you could add some tests that would be perfect. |
|
Great. Thanks for being so responsive. I'll add some tests and get this merged as soon as I can. |
|
@jharding: bump |
|
@jharding: any news? It's been 9 months :/ |
|
Would really love to be able to use |
|
Branch rebased; I updated dependencies including lodash itself. |
|
@XhmikosR What do you think about just vendoring this task into Bootstrap's |
|
I was thinking to use our own fork, but that would work too, sure. |
|
@XhmikosR I'm fine with either approach; just want to get this finally resolved soon 😅 . Think you'll have time to setup the fork in the near future? |
This fixes #9.
I'll rebase this when the other PR is merged.