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

gocognit lintering warnings for Range support #278

Closed
atc0005 opened this issue Oct 25, 2024 · 2 comments · Fixed by #290
Closed

gocognit lintering warnings for Range support #278

atc0005 opened this issue Oct 25, 2024 · 2 comments · Fixed by #290
Assignees
Labels
Milestone

Comments

@atc0005
Copy link
Owner

atc0005 commented Oct 25, 2024

Overview

Output from a recent CI run against the development branch:

golangci-lint has version 1.61.0 built with go1.23.1 from a1d6c560 on 2024-09-09T17:44:42Z
level=warning msg="The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar."
range.go:147:1: cognitive complexity 17 of func `(*Plugin).EvaluateThreshold` is high (> 10) (gocognit)
func (p *Plugin) EvaluateThreshold(perfData ...PerformanceData) error {
^
range.go:49:1: cognitive complexity 11 of func `(Range).checkOutsideRange` is high (> 10) (gocognit)
func (r Range) checkOutsideRange(valueAsAFloat float64) bool {
^
range.go:79:1: cognitive complexity 11 of func `ParseRangeString` is high (> 10) (gocognit)
func ParseRangeString(input string) *Range {
^
Error: Process completed with exit code 1.

Relevant bits:

range.go:147:1: cognitive complexity 17 of func `(*Plugin).EvaluateThreshold` is high (> 10) (gocognit)
func (p *Plugin) EvaluateThreshold(perfData ...PerformanceData) error {
^

range.go:49:1: cognitive complexity 11 of func `(Range).checkOutsideRange` is high (> 10) (gocognit)
func (r Range) checkOutsideRange(valueAsAFloat float64) bool {
^

range.go:79:1: cognitive complexity 11 of func `ParseRangeString` is high (> 10) (gocognit)
func ParseRangeString(input string) *Range {
^

While these warnings have been emitted for a while now they don't impact functionality so haven't been a priority. I'd like to resolve them as part of the next release.

References

@atc0005 atc0005 added bug Something isn't working linting range thresholds labels Oct 25, 2024
@atc0005 atc0005 added this to the v0.17.0 milestone Oct 25, 2024
@atc0005 atc0005 self-assigned this Oct 25, 2024
@atc0005
Copy link
Owner Author

atc0005 commented Oct 26, 2024

@atc0005
Copy link
Owner Author

atc0005 commented Oct 31, 2024

range.go:49:1: cognitive complexity 11 of func `(Range).checkOutsideRange` is high (> 10) (gocognit)
func (r Range) checkOutsideRange(valueAsAFloat float64) bool {
^

I worked with this method for a bit, then turned to popular LLMs for alternatives. Those suggestions were much better than mine and appear to improve on the original. The Google Gemini suggestion appears to be the easiest to grok, so will plan to use it to replace the original.

Google Gemini (model 'Gemini 1.5 Flash')

func (r Range) checkOutsideRange(valueAsAFloat float64) bool {
	// Explanation of the Simplification:
	//
	// Infinite Bounds:
	//
	// If the start is infinite, the value is outside the range only if it's
	// greater than the end. If the end is infinite, the value is outside the
	// range only if it's less than the start.
	//
	// Finite Bounds:
	//
	// The value is outside the range if it's either less than the start or
	// greater than the end.

	// Handle infinite bounds first
	if r.StartInfinity {
		return valueAsAFloat > r.End
	} else if r.EndInfinity {
		return valueAsAFloat < r.Start
	}

	// Handle finite bounds
	return valueAsAFloat < r.Start || valueAsAFloat > r.End
}

I moved the explanation notes within the function for reference.

ChatGPT (model 'GPT-4o')

func (r Range) checkOutsideRange(value float64) bool {
	// Explanation of the Simplification:
	//
	// Each case is now focused only on the conditions that make the value
	// outside the range. The final default case covers the fully infinite
	// range, simplifying the logic to just return false since no bounds
	// restrict the range.

	switch {
	case !r.StartInfinity && value < r.Start:
		return true
	case !r.EndInfinity && value > r.End:
		return true
	case r.StartInfinity && r.EndInfinity:
		return false
	default:
		return false
	}
}

As before, I placed the explanation for the changes within the function for reference.

atc0005 added a commit that referenced this issue Oct 31, 2024
Replace existing method with implementation provided by Google
Gemini (model 'Gemini 1.5 Flash').

Include commented out alternative implementation from ChatGPT
(model 'GPT-4o') for contrast.

refs GH-278
atc0005 added a commit that referenced this issue Oct 31, 2024
Rework existing function with suggested changes from ChatGPT
(model 'GPT-4o') with an additional small adjustment to reduce
the gocognit linter score further.

As described by ChatGPT:

> Organizational Improvements:
>
> Each parsing step is now grouped by function (alert inversion,
> infinity handling, start parsing, and end parsing), making it
> easier to follow while keeping the parsing steps in their
> original form.
>
> Retained Validation Checks:
>
> The final if check ensures range validity before returning the
> struct.

refs GH-278
atc0005 added a commit that referenced this issue Oct 31, 2024
Replace existing method with implementation provided by ChatGPT
(model 'GPT-4o'), including closure which was moved to separate
`evaluateThreshold` helper function.

refs GH-278
atc0005 added a commit that referenced this issue Nov 6, 2024
Replace existing method with implementation provided by Google
Gemini (model 'Gemini 1.5 Flash').

Include commented out alternative implementation from ChatGPT
(model 'GPT-4o') for contrast.

refs GH-278
atc0005 added a commit that referenced this issue Nov 6, 2024
Rework existing function with suggested changes from ChatGPT
(model 'GPT-4o') with an additional small adjustment to reduce
the gocognit linter score further.

As described by ChatGPT:

> Organizational Improvements:
>
> Each parsing step is now grouped by function (alert inversion,
> infinity handling, start parsing, and end parsing), making it
> easier to follow while keeping the parsing steps in their
> original form.
>
> Retained Validation Checks:
>
> The final if check ensures range validity before returning the
> struct.

refs GH-278
atc0005 added a commit that referenced this issue Nov 6, 2024
Replace existing method with implementation provided by ChatGPT
(model 'GPT-4o'), including closure which was moved to separate
`evaluateThreshold` helper function.

refs GH-278
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant