Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add affordance for coverage flag stack #904

Closed
skeggse opened this issue Feb 8, 2019 · 7 comments · Fixed by #926
Assignees
Labels
Milestone

Comments

@skeggse
Copy link
Contributor

@skeggse skeggse commented Feb 8, 2019

If I configure a transform using -T with babel, I might want to have babel comment out portions of the code it generates with auxiliaryCommentBefore and auxiliaryCommentAfter so that lab doesn't try to lint the generated code. Separately, I might sometimes want to disable coverage for actual application code. If I put the two together, it's possible that a block of code that has linting disabled gets re-enabled by the generated code's comments:

// input:
/* $lab:coverage:off$ */
const { types } = Util;
export const isSet = (types && types.isSet) || internals.isSet;
/* $lab:coverage:on$ */



// output:
/* $lab:coverage:off$ */
const {
  types
} =
/*$lab:coverage:off$*/
_util
/*$lab:coverage:on$*/
.
/*$lab:coverage:off$*/
default
/*$lab:coverage:on$*/
;
const isSet = types && types.isSet || internals.isSet;        // This line gets linted, even though it shouldn't have been.
/* $lab:coverage:on$ */

I'm curious whether there's an existing solution to this problem.

If not, would we be open to adding support for /* $lab:coverage:push$ */ and /* $lab:coverage:pop$ */ comments (name unimportant), which would create a running stack of flag states that auto-generated code could use to avoid mucking with the user's intended coverage state?

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Feb 11, 2019

@skeggse thanks for raising this. In the case of the above example how would you use the push and pop to solve the problem? It sounds like a good approach, I'd just like to see an example with it being used.

@skeggse

This comment has been minimized.

Copy link
Contributor Author

@skeggse skeggse commented Feb 11, 2019

I was imagining it looking something like this:

Input

/* $lab:coverage:off$ */
const { types } = Util;
export const isSet = (types && types.isSet) || internals.isSet;
/* $lab:coverage:on$ */

Output

/* $lab:coverage:off$ */
const {
  types
} =
/*$lab:coverage:push$/
/*$lab:coverage:off$*/
_util
/*$lab:coverage:pop$/
.
/*$lab:coverage:push$/
/*$lab:coverage:off$*/
default
/*$lab:coverage:pop$*/
;
const isSet = types && types.isSet || internals.isSet;
/* $lab:coverage:on$ */

All of the above occurrences of $lab:coverage:pop$ would restore the state of the coverage flag from the corresponding push$, and set the state back to off. In more interesting cases, multiple tools might be manipulating the source and interleaving multiple operations where the coverage could turn on and off "inside" an existing pushed state.

This is kinda inspired by #pragma pack(push, xx) (with corresponding #pragma pack(pop, xx)) but a little lighter-weight.

EDIT: we could also parameterize push, like so: /*$lab:coverage:push off*/, which would take the place of a push$ statement followed by a off$ statement.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Feb 13, 2019

@skeggse this looks like a fine approach to the problem. Thank you for explaining it with an example. I'm open to this solution and getting it merged in and released.

Thanks!

@skeggse

This comment has been minimized.

Copy link
Contributor Author

@skeggse skeggse commented Feb 14, 2019

Sure! I'm happy to take a stab at this - unless you've already started on it?

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Feb 14, 2019

@skeggse I haven't started yet :/ Thank you for being willing to take a stab at it!

@skeggse

This comment has been minimized.

Copy link
Contributor Author

@skeggse skeggse commented Feb 15, 2019

Cool, no problem! I was mostly trying to understand whether your previous comment was a commitment to implementing this yourself or just an expression of interest in this solution becoming part of lab.

@skeggse skeggse assigned skeggse and unassigned geek Feb 23, 2019
@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented May 12, 2019

Ping to make sure this is still something you want to work on.

@geek geek closed this in #926 Jun 14, 2019
@geek geek modified the milestones: 19.1.1, 19.1.0 Jun 14, 2019
@Marsup Marsup added feature and removed request labels Sep 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.