Sorting result.Issues implementation (golangci/golangci-lint#1217)

[butuzov]

Implements #1217

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[sayboras]

Common way is just comparing to zero. We can simplify consts as well.

[butuzov]

Same reasons as in res.isNeutral() related comment. Read it as a story: If the results of comparing a and b state Less, compare function receives true. I would keep names constants as it really simplifies the understanding of the code for a small price.

[sayboras]

This is tricky one. Will it be "<=" instead of "==" ?

I didn't scan for all Comparator implementation to make sure that negative result will only be -1

[sayboras]

understand that your intention is to compose the sorting rules. However, if I looked at the Comparer we have now ByName along.

               ByName{
			next: ByLine{
                        ...
			},
		}

Comparing ByName implicitly checking Line number due to composition, but will ByName comparator should only deal with names, it doesn't require to know the next rule should be.

[daixiang0]

+1

[butuzov]

This is Chain of Responsibility pattern, current resolver only needs to know about the exitance of the next one (res.isNeutral() we calling defines should we proceed to the next rule or not, but rules are different, strings compare, single numbers compare (line and column), two numbers(range) ). Guess we suppose to see the Comparator interface, instead of chain of its implementations. ByName or any other implementation of Comparator needs to know only that there is the next rule in the chain.

[sayboras]

Not sure if chain of responsibility pattern is correct one to use here. Maybe I am mixing two different things (comparator interface + design pattern), but if we can interface as Comparator, it should just to compare two entities and nothing else.

The logic of calling next comparator is kind of repetitive in currently (e.g. check res is neutral or not, if not check the next one is there, then call next one), so adding new Comparator will require a fairly some level of understanding of existing one.

Few snippets (based on your code) to illustrate my thinking:

type SortResults struct {
	compareFuncs []compareFunc
	cfg          *config.Config
}

type compareFunc func(result.Issue, result.Issue) compareResult

func NewSortResults(cfg *config.Config) *SortResults {
        // single responsibility function  
	byName := func(first result.Issue, second result.Issue) compareResult {
		return compareResult(strings.Compare(first.FilePath(), second.FilePath()))
	}

	byLine := func(first result.Issue, second result.Issue) compareResult {
		return compareResult(first.Line() - second.Line())
	}

	byCol := func(first result.Issue, second result.Issue) compareResult {
		return compareResult(first.Column() - second.Column())
	}

	byRange := func(first result.Issue, second result.Issue) compareResult {
		return compareResult(first.GetLineRange().From - second.GetLineRange().From)
	}

	return &SortResults{
		compareFuncs: []compareFunc{
			byName,
			byLine,
			byCol,
			byRange,
                        ... --> we can add more type of compareFunc here
		},
		cfg: cfg,
	}
}

// Process is performing sorting of the result issues.
func (sr SortResults) Process(issues []result.Issue) ([]result.Issue, error) {
	if !sr.cfg.Output.SortResults {
		return issues, nil
	}

	sort.Slice(issues, func(i, j int) bool {
		for _, c := range sr.compareFuncs {
			if res := c(issues[i], issues[j]); !res.isNeutral() {
				return res <= less
			}
		}
		return false
	})
	return issues, nil
}

[butuzov]

I had to use CoR in order to avoid making logic complicated (cmon who what to see 3-5 blocks of different if statements ?), CoR helps to pass the request along a chain of handlers. More of that - I can test each of them. And logic isn't really simple here (what if linter don't provide column? or line?) I did try to cover it with tests.

func numericCompare(a, b int) CompareResult {
	var (
		isValuesInvalid  = a < 0 || b < 0
		isZeroValuesBoth = a == 0 && b == 0
		isEqual          = a == b
		isZeroValueInA   = b > 0 && a == 0
		isZeroValueInB   = a > 0 && b == 0
	)

	switch {
	case isZeroValuesBoth || isEqual:
		return Equal
	case isValuesInvalid || isZeroValueInA || isZeroValueInB:
		return None
	case a > b:
		return Greater
	case a < b:
		return Less
	}

	return Equal
}

I see what you have written, and got the idea, and I wish I have more time for opensource, to rethink the structure and implement all checks, write new tests, cover edge cases to pleasure community, and maintainers in order to keep code quality high. But my family wouldn't agree with me. I am really sorry for wasting your time with this PR. Please reject this PR if you think code quality is way lower the level you will accept.

[sayboras]

I see what you have written, and got the idea, and I wish I have more time for opensource, to rethink the structure and implement all checks, write new tests, cover edge cases to pleasure community, and maintainers in order to keep code quality high. But my family wouldn't agree with me. I am really sorry for wasting your time with this PR. Please reject this PR if you think code quality is way lower the level you will accept.

Appreciate that you spend time to send PR with a very good coverage 💯 . Yeah, I personally understand your point about OSS involvement. While only few are lucky to have OSS as paid job, the rest of us (including myself) are voluntarily doing it either during night, weekends or our own spare time. In my opinion, OSS PR process is a chance to have open discussion, and in turn learn from each other. Sometimes it can take a little longer than we expect.

I could be a bit opinionated, or even wrong on this subject. So, let me park it right here, we can see if someone else from @golangci/team has any comment/idea in general.

In the meantime, I can continue reviewing on the basis that CoR is used. Hope it is fine to you.

[butuzov]

Appreciate this. Using your way to solve the issue will require additional time to think about implementation and tests, and actually require to throw my code out almost entirely.

[sayboras]

I have sometime to finish review now. Feel free to address when you see fit.

PS: Can you add something like Fixes https://github.com/golangci/golangci-lint/issues/1217 in PR description, so that underlying issue and PR can be linked together? Github didn't scan in PR title.

[sayboras]

Added few last comments on run_tests and private const/structs.

LGTM on basis that CoR is used.

[sayboras]

this could be private

[butuzov]

done, I also needed to add //nolint:golint directive to suppress warning on false positive message about returning unexported value.

[sayboras]

this could be private

[butuzov]

I know it's controversial, but can we keep constants at least CamelCased?

[sayboras]

nit: this could be private

[butuzov]

done

[sayboras]

nit: these comparator can be changed to private

[butuzov]

done

[sayboras]

can you add the test for this option?

[SVilgelm]

I think, @sayboras means to add the tests in the test/run_test.go because of the new config option

[butuzov]

@SVilgelm @sayboras added, in addition, changed help text.

[SVilgelm]

Looks good to me. Thank you
Tests are passed with rebasing over master branch

[golangci-automator]

Hey, @butuzov — we just merged your PR to golangci-lint! 🔥🚀

golangci-lint is built by awesome people like you. Let us say “thanks”: we just invited you to join the GolangCI organization on GitHub.
This will add you to our team of maintainers. Accept the invite by visiting this link.

By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.
More information about contributing is here.

Thanks again!

[butuzov]

Thank everyone, that was quite a ride =)

[sayboras]

Thank everyone, that was quite a ride =)

haha hope you enjoy it 😅. Looking forward to your next ride then 👍