priv-kweihmann / oelint-adv

Advanced oelint
BSD 2-Clause "Simplified" License
53 stars 27 forks source link

Whitespace around operation #563

Closed UVV-gh closed 3 months ago

UVV-gh commented 3 months ago

I've encountered an issue with oelint-adv that an assignment with extra whitespace is not parsed correctly.

The recipe like:

VAR1 = "A"
VAR1  += "B"
VAR2  += "C"

is reported as false positive in the second line. I assume that has something to do with this regexp

UVV-gh commented 3 months ago

I'd assume the fix might be:

-   __regex_var = r"^(?P<varname>([A-Z0-9a-z_.-]|\$|\{|\}|:)+?)(?P<varop>(\s|\t)*(\+|\?|\:|\.)*=(\+|\.)*(\s|\t)*)(?P<varval>.*)"
+   __regex_var = r"^(?P<varname>([A-Z0-9a-z_.-]|\$|\{|\}|:)+?)(?P<varop>(\s|\t)(\+|\?|\:|\.)*=(\+|\.)*(\s|\t))(?P<varval>.*)"

What should be the way to integrate it into the PR test pipeline? I'm asking because the test case would actually be in the oelint-adv project.

priv-kweihmann commented 3 months ago

Hmmm, how is that wrong?

With the original regex all of the following is parsed correctly

VAR1 = "A"
VAR1  += "B"
VAR2  += "C"
VAR2+= "C"

while with your patch only the first line will be caught, so I honestly think it makes it much worse.

I'm having a hard time to understand how VAR2<space><space>+= "C" is not wrong, the assignment needs to be VAR2<space>+= "C", a quick test returns

test.bb:2:warning:oelint.vars.spacesassignment:Suggest spaces around assignment. E.g. 'FOO = "BAR"'
test.bb:3:warning:oelint.vars.spacesassignment:Suggest spaces around assignment. E.g. 'FOO = "BAR"

which is the expected result IMO.

Could you please elaborate what you think the false positive is

UVV-gh commented 3 months ago

Yes, it is parsed correctly, but VarOp is going to be += in that case, that returns "error:oelint.var.override:Variable 'A' is set by 'recipe.bb'" for the first <space><space>+= line. I agree that it's a strange formatting, but bitbake is OKish with that. Does it make sense? :)

You can reproduce it with this test case

I couldn't find in the docs that such assignment is not valid. When it's not, then the error message should hint at it (for now it's ambiguous about it).

priv-kweihmann commented 3 months ago

ahhh now I see what the false positive is, so it ain't about the parser but the particular rule - will try to fix that in one of the next release (and also moving this ticket to the linter, as it's not an issue of the parser)