Adds new lost-vars functionality. #389
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #389 +/- ##
===========================================
+ Coverage 91.66% 92.02% +0.35%
===========================================
Files 16 19 +3
Lines 672 702 +30
===========================================
+ Hits 616 646 +30
Misses 56 56
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Gah! I'm in love with this code!
- Brilliant how you have
lgGutterextend the logic of this! - The
lost-vars()logic is really nice. It'll be really nice to expand on! - There looks like possible duplicate tests. (see comment)
- It'd be nice to have a test for both
gutterandgutter-localin one declaration. (see comment) - Do we want to add a warning to the existing
lgLogicimplementation since we're planning on deprecating it in the future?
That's all I can think of right now. This is awesome stuff!
test/lost-vars.js
Outdated
| ); | ||
| }); | ||
| it('allows for multiple uses of the variable', () => { | ||
| check( |
There was a problem hiding this comment.
Is this a duplicate of the test on ln15?
There was a problem hiding this comment.
It is, great minds again! I'd done one of my own and then repurposed your tests, which had one too. I've removed this one as it was specific to just gutter-local. Whereas the other one covers both.
| ); | ||
| }); | ||
|
|
||
| it('matches multiple variables', function() { |
There was a problem hiding this comment.
It might also be good to have a test for the use of both the lost-vars('gutter') and lost-vars('gutter-local') in one declaration.
There was a problem hiding this comment.
I've updated this test to include a new check which does just that.
lib/lost-vars.js
Outdated
|
|
||
| var variableFunctions = { | ||
| 'gutter': lostGutter, | ||
| 'gutter-local': lostGutterLocal |
There was a problem hiding this comment.
A hanging comma on this would help with future diffs.
| } | ||
|
|
||
| variables.forEach(variable => { | ||
| var func = variableFunctions[variable]; |
There was a problem hiding this comment.
I'm in love with this code!
There was a problem hiding this comment.
Thanks, I'm really enjoying working on Lost :)
|
What release will include this feature? |
|
At this point, since it's done I don't see why it has to wait for 8.3. I'm the holdup on 8.2 and getting the test coverage up #381. 😳 |
What kind of change is this? (Bug Fix, Feature...)
Feature.
What is the current behavior (You can also link to an issue)
Solves #376. Future replacement for
$lost-gutterand$lost-gutter-localvariables.What is the new behavior this introduces (if any)
New
lost-varsmethod to output variables.Does this introduce any breaking changes?
No.
Does the PR fulfill these requirements?
Other Comments
I've updated
lg-gutterso that it shares same functionality as newlost-varsmethod, save duplicating code. Only thing I'm not sure you'll like is how I've broken out each of the new variable methods into its own file. See what you think, I can always change it if you aren't keen.