Skip to content

Update rule check by string rather than loop#11

Open
rogeruiz wants to merge 1 commit intoapuigsech:masterfrom
rogeruiz:check-rule-by-string
Open

Update rule check by string rather than loop#11
rogeruiz wants to merge 1 commit intoapuigsech:masterfrom
rogeruiz:check-rule-by-string

Conversation

@rogeruiz
Copy link

@rogeruiz rogeruiz commented Oct 6, 2016

This patch updates the rule check to happen on the string that is being
iterated on by inspect_worker().

Address issue #10

This patch updates the rule check to happen on the string that is being
iterated on by `inspect_worker()`.

Address issue apuigsech#10
@apuigsech
Copy link
Owner

apuigsech commented Oct 10, 2016

What do you want to solve with this change?

I see some problems with it;

  • Will not be possible to create multi-line rules.
  • Will not be possible (if in the future we do) to create a more complex rules definition that analizes the context deeply using information from previous/following lines.

In general I don't see any reason to make this change.

@LinuxBozo
Copy link

@apuigsech
Your question about what do you want solve with this change is pretty simple, and referenced already: Fix #10

As for your problems:

  1. This is not correct. Check the code again. This has no affect on rules at all. Just the data fed into the rule for matching.
  2. This is more correct, but would require more changes anyway since you are already using scan() on the buffer which only returns a single line here: 006b2c9#diff-8a696d055b78243df5ebf6c2a1fb41a8L76 The problem then becomes, since you are only feeding the single line into the rules Run, and that is where you have also decided to emit the line number back to the reporting mechanism via RunResult, all errors occur on line 1. Always. So again, this fixes Line numbers not correct #10.

This code can always be revisited if/when you find there is a need for this deep inspection, but I have a feeling that by that time, you will want to completely refactor some of this, and this change becomes a non-issue anyway.

@LinuxBozo
Copy link

@apuigsech any more thoughts on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants