Skip to content

chore: change main()'s retval behavior #71

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

chore: change main()'s retval behavior #71

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6792ff
Change main()'s retval behavior; fix cli test
airween Sep 26, 2025
0333b42
Remove duplicated case check
airween Sep 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/crs_linter/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,7 @@ def main():

error = check_indentation(f, parsed[f])
if error:
retval = 1
retval |= 1 << len(c.error_vars)

### check `ctl:auditLogParts=+E` right place in chained rules
c.check_ctl_audit_log()
Expand Down Expand Up @@ -577,15 +577,15 @@ def main():
file=f,
title="no tests",
)
retval = 1

# set it once if there is an error
if c.is_error():
errno = c.is_error()
if errno != 0:
logger.debug(f"Error(s) found in {f}.")
retval = 1
retval |= errno

logger.end_group()
if c.is_error() and logger.output == Output.GITHUB:
if errno != 0 and logger.output == Output.GITHUB:
# Groups hide log entries, so if we find an error we need to tell
# users where it is.
logger.error("Error found in previous group")
Expand All @@ -596,6 +596,7 @@ def main():
for tk in txvars:
if not txvars[tk]["used"]:
if not has_unused:
retval |= 1 << len(c.error_vars)+1
logger.debug("Unused TX variable(s):")
a = txvars[tk]
logger.error(
Expand Down
14 changes: 10 additions & 4 deletions src/crs_linter/linter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,7 @@ def __init__(self, data, filename=None, txvars={}):
self.re_fname = re.compile(r"(REQUEST|RESPONSE)\-\d{3}\-")
self.filename_tag_exclusions = []

def is_error(self):
"""Returns True if any error is found"""
error_vars = [
self.error_vars = [
self.error_case_mistmatch,
self.error_action_order,
self.error_wrong_ctl_auditlogparts,
Expand All @@ -142,8 +140,16 @@ def is_error(self):
self.error_no_crstag,
self.error_no_ver_action_or_wrong_version,
self.error_tx_N_without_capture_action,
self.error_rule_hasnotest
]
return any([len(var) > 0 for var in error_vars])

def is_error(self):
"""Returns non-zero if any error is found, 0 otherwise."""
errno = 0
for eidx in range(0, len(self.error_vars)):
if len(self.error_vars[eidx]) > 0:
errno |= 1 << eidx
return errno

def store_error(self, msg):
# store the error msg in the list
Expand Down
8 changes: 4 additions & 4 deletions tests/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ def test_cli(monkeypatch, tmp_path):
approved_tags = tmp_path / "APPROVED_TAGS"
test_exclusions = tmp_path / "TEST_EXCLUSIONS"
approved_tags.write_text("")
test_exclusions.write_text("")
test_exclusions.write_text("901120\n901125\n901141\n920160\n920161\n920162")

monkeypatch.setattr(
sys,
Expand All @@ -17,9 +17,9 @@ def test_cli(monkeypatch, tmp_path):
"-v",
"4.10.0",
"-r",
"../examples/test1.conf",
"examples/test1.conf",
"-r",
"../examples/test?.conf",
"examples/test?.conf",
"-t",
str(approved_tags),
"-T",
Expand All @@ -33,4 +33,4 @@ def test_cli(monkeypatch, tmp_path):

ret = main()

assert ret == 0
assert ret == 26367
Copy link
Member

@fzipi fzipi Sep 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come we know that 26367 means error X or Y? 🤣

Copy link
Contributor Author

@airween airween Sep 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your question is absolutely valid.

First of all: during the review I found a bug. There was a duplicated check which used one more bit than it was necessary, so now the expected value is not 26367 but 9983.

9983 in binary format is 10 0110 1111 1111. The highest value is 2^(14-1).

Here you can see the listed error containers. As you can see there are 13 containers. If any container contains an error then its bit will turn on to 1: the first container (error_case_mistmatch) is 2^0 := 1, the second (error_action_order) is 2^1 := 2 and so on. The last (13th) one is the error_rule_hasnotest, its value can be 2^(13-1) := 4096.

There is one more possible error: the intendation error. This is a pre-check, it runs before the linter, so this "container" is not part of the linter class, and I didn't want to mix that. The most simple solution was to append at the end - this is why it uses len(c.error_vars).

Here is the full table:

Error Value
case mismatch error 1
action order error 2
ctl:auditlogparts in wrong place 4
undefined TX variable 8
inconsistent PL tag 16
inconsistent PL score 32
duplicate id 64
unknown tag 128
combined transformation and ignorecase 256
no tag:CRS... 512
no ver action or wrong value 1024
rule uses tx:N without capture 2048
rule has no test 4096
indentation error 8192

The last item is implicit an one, all others are explicit. This means if we extend the type of containers, the indentation error will have a new value.

The value 9983 = case mismatch error | action order error | ctl:auditlogparts in wrong place | undefined TX variable | inconsistent PL tag | inconsistent PL score | duplicate id | unknown tag | no tag:CRS... | no ver action or wrong value | indentation error.

Error flag "combined transformation and ignorecase", "rule uses tx:N without capture" and "rule has no test" flags aren't set. None the any rule has tests, but I filled the missing id's in linter's test so the last one is not set because of this.

I know this is impenetrable and hard to see but it's absolutely coherent and de-composable. I think this isn't necessary to understand, I just add this modification because here I offered a solution which was accepted by @theseion.

I can add some more code to make a map where we can check assertion's value not by an explicit integer, but a bit mask which compounded by flags, eg.

    assert ret = linter.ERR_CASE_MISMATCH | linter.ERR_ACTION_ORDER | ...

Would that be better?

Copy link
Member

@fzipi fzipi Sep 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the bitwise form of returning errors, just trying to understand this need. Because we didn't returned all the errors before, so now we need to interpret the results in a different way.

It is better to have a map at least to have it easy to read.

Loading