-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Regexp directives #2
Comments
When I offered the suggestion in the first place, my thinking was that the default mode of the tool was to automatically include all functions. That would be the equivalent of these default settings:
These two directives could be run first to establish the initial instrument function table. And then any user directives would act upon filtering this instrument table, vs all available functions. The evaluation should always run the users exclude rules first, and then the include rules. This default case then builds an instrument table containing all functions. This means the user could then exclude some:
Now if the user wants to invert the default mode and exclude all, but include some, they just need to define the exclude to clear the table and then add back in a subset:
As far as I can tell, this shouldn't collide between the logic of two modes you mentioned. The system should always start with an include-all approach, and then apply all user excludes first and then includes second. |
Hi,
Yeah, I think this can work. Using directive is a func of form
`(map[str]bool) -> map[str]bool` and chaining them in sequence they appear
in file may work.
Good suggestion!
…On Sat, 26 Nov 2022 at 02:55, Justin Israel ***@***.***> wrote:
When I offered the suggestion in the first place, my thinking was that the
default mode of the tool was to automatically include all functions. That
would be the equivalent of these default settings:
instrument:exclude
instrument:include .*
These two directives could be run first to establish the initial
instrument function table. And then any user directives would act upon
filtering this instrument table, vs all available functions.
The evaluation should always run the users exclude rules first, and then
the include rules. This default case then builds an instrument table
containing all functions.
This means the user could then exclude some:
//instrument:exclude Some
Now if the user wants to invert the default mode and exclude all, but
include some, they just need to define the exclude to clear the table and
then add back in a subset:
//instrument:exclude .*
//instrument:include Some
As far as I can tell, this shouldn't collide between the logic of two
modes you mentioned. The system should always start with an include-all
approach, and then apply all user excludes first and then includes second.
—
Reply to this email directly, view it on GitHub
<#2 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAWMCRO3SPWEA7BRQPWN3OLWKEDS7ANCNFSM6AAAAAASLJHUX4>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Only one issue left — ordering of directives. To use this approach with
regex, we have to enforce that directives have to be applied in order.
Current version, does not restrict so — directives can be in any order
anywhere in file. A bit hard to tell how people use it: put all at the top?
spread all over the file? add directive next to function? So I would keep
current version and give it a time.
Lets keep this suggestion tho!
On Sat, 26 Nov 2022 at 05:16, Nikolay Dubina ***@***.***>
wrote:
… Hi,
Yeah, I think this can work. Using directive is a func of form
`(map[str]bool) -> map[str]bool` and chaining them in sequence they appear
in file may work.
Good suggestion!
On Sat, 26 Nov 2022 at 02:55, Justin Israel ***@***.***>
wrote:
> When I offered the suggestion in the first place, my thinking was that
> the default mode of the tool was to automatically include all functions.
> That would be the equivalent of these default settings:
>
> instrument:exclude
> instrument:include .*
>
> These two directives could be run first to establish the initial
> instrument function table. And then any user directives would act upon
> filtering this instrument table, vs all available functions.
>
> The evaluation should always run the users exclude rules first, and then
> the include rules. This default case then builds an instrument table
> containing all functions.
>
> This means the user could then exclude some:
>
> //instrument:exclude Some
>
> Now if the user wants to invert the default mode and exclude all, but
> include some, they just need to define the exclude to clear the table and
> then add back in a subset:
>
> //instrument:exclude .*
> //instrument:include Some
>
> As far as I can tell, this shouldn't collide between the logic of two
> modes you mentioned. The system should always start with an include-all
> approach, and then apply all user excludes first and then includes second.
>
> —
> Reply to this email directly, view it on GitHub
> <#2 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAWMCRO3SPWEA7BRQPWN3OLWKEDS7ANCNFSM6AAAAAASLJHUX4>
> .
> You are receiving this because you modified the open/close state.Message
> ID: ***@***.***>
>
|
+1 to this: do people want to "exclude-all/include-all" specified per file?
If so, then may need this solution.
On Sat, 26 Nov 2022 at 05:45, Nikolay Dubina ***@***.***>
wrote:
… Only one issue left — ordering of directives. To use this approach with
regex, we have to enforce that directives have to be applied in order.
Current version, does not restrict so — directives can be in any order
anywhere in file. A bit hard to tell how people use it: put all at the top?
spread all over the file? add directive next to function? So I would keep
current version and give it a time.
Lets keep this suggestion tho!
On Sat, 26 Nov 2022 at 05:16, Nikolay Dubina ***@***.***>
wrote:
> Hi,
>
> Yeah, I think this can work. Using directive is a func of form
> `(map[str]bool) -> map[str]bool` and chaining them in sequence they appear
> in file may work.
>
> Good suggestion!
>
> On Sat, 26 Nov 2022 at 02:55, Justin Israel ***@***.***>
> wrote:
>
>> When I offered the suggestion in the first place, my thinking was that
>> the default mode of the tool was to automatically include all functions.
>> That would be the equivalent of these default settings:
>>
>> instrument:exclude
>> instrument:include .*
>>
>> These two directives could be run first to establish the initial
>> instrument function table. And then any user directives would act upon
>> filtering this instrument table, vs all available functions.
>>
>> The evaluation should always run the users exclude rules first, and then
>> the include rules. This default case then builds an instrument table
>> containing all functions.
>>
>> This means the user could then exclude some:
>>
>> //instrument:exclude Some
>>
>> Now if the user wants to invert the default mode and exclude all, but
>> include some, they just need to define the exclude to clear the table and
>> then add back in a subset:
>>
>> //instrument:exclude .*
>> //instrument:include Some
>>
>> As far as I can tell, this shouldn't collide between the logic of two
>> modes you mentioned. The system should always start with an include-all
>> approach, and then apply all user excludes first and then includes second.
>>
>> —
>> Reply to this email directly, view it on GitHub
>> <#2 (comment)>,
>> or unsubscribe
>> <https://github.com/notifications/unsubscribe-auth/AAWMCRO3SPWEA7BRQPWN3OLWKEDS7ANCNFSM6AAAAAASLJHUX4>
>> .
>> You are receiving this because you modified the open/close state.Message
>> ID: ***@***.***>
>>
>
|
My opinion is that you shouldn't need to guarantee order across files. It should just collect them all into exclude and include maps and then run all the excludes and then all the includes. If users want to spread them all around then they should just be aware not to do very wide nets on their regex and just use them to include one off functions. Edit: to be clear, I mean per package, like the way imports work. |
suggested:
issues:
The text was updated successfully, but these errors were encountered: