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

report when overall threshold present (or) overrides are present #141

Merged
merged 4 commits into from
Jan 24, 2025

Conversation

subhambhardwaj
Copy link
Contributor

@subhambhardwaj subhambhardwaj commented Jan 23, 2025

Changes:
Allow reports of pkg and file level overrides even when overall pkg and file thresholds are 0

Closes: #140

Copy link
Owner

@vladopajic vladopajic left a comment

Choose a reason for hiding this comment

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

would it be possible to have two flags HasFileOverrides and HasPackageOverrides ?


return AnalyzeResult{
Threshold: thr,
HasOverrides: hasOverrides,
Copy link
Owner

Choose a reason for hiding this comment

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

compileOverridePathRules does not need to return additional value, instead we can use something like this:

HasOverrides:        len(overrideRules) > 0,

@subhambhardwaj
Copy link
Contributor Author

would it be possible to have two flags HasFileOverrides and HasPackageOverrides ?

I have made the changes for this, please have a look

Copy link
Owner

@vladopajic vladopajic left a comment

Choose a reason for hiding this comment

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

detectOverrides seems quite good direction, please have a look at comments.

also make sure to check make lint, make test and make check-coverage when you are done

Comment on lines 81 to 83
if len(cfg.Override) > 0 {
hasFileOverrides, hasPackageOverrides = detectOverrides(cfg.Override)
}
Copy link
Owner

Choose a reason for hiding this comment

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

if len(cfg.Override) > 0 { seems unnecessary, detectOverrides should return correct result for any size of cfg.Override

Comment on lines 101 to 106
if strings.HasSuffix(override.Path, ".go") {
hasFileOverrides = true
}
if strings.HasPrefix(override.Path, "^") {
hasPackageOverrides = true
}
Copy link
Owner

Choose a reason for hiding this comment

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

would it make sense to have these if statements like this:

if is file override {
  ...
} else {
 if it is not file override then it must be package override
}

also file overrides may have .go$ suffix, for example here .

Copy link
Contributor Author

@subhambhardwaj subhambhardwaj Jan 24, 2025

Choose a reason for hiding this comment

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

I updated the check here to

if override name contains ".go" suffix || ".go$" suffix {
    // file override
} else {
    // pkg override
}

if strings.HasPrefix(override.Path, "^") {
hasPackageOverrides = true
}
if hasFileOverrides && hasPackageOverrides {
Copy link
Owner

Choose a reason for hiding this comment

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

i don't have anything against of merging this if, but it seems unnecessary. these overrides are usually very small, lets say there is 100 of them (99.99% of users will have less then 10). with this size, this for cycle will finish in 1ns or less, so this if wont produce any noticeably benefits. and on another hand because this repo requires 100% coverage, this if will required to have additional tests case that needs to be added or ignored.

@subhambhardwaj
Copy link
Contributor Author

I checked make test, make lint and make check-coverage

coverage stats are

File coverage threshold (100%) satisfied:       PASS
Total coverage threshold (98%) satisfied:       PASS
Total test coverage: 98.4% (505/513)

@vladopajic vladopajic merged commit 6ffd174 into vladopajic:main Jan 24, 2025
3 of 7 checks passed
@vladopajic
Copy link
Owner

@subhambhardwaj thanks for the effort! i'll make new release later today

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.

reports are added to output only if thresholds are specified
3 participants