Fix Go Report Card Issues #131

HakanVardarr · 2024-03-30T06:53:11Z

I fixed all of the issues listed on goreportcard.

ccoVeille

As I said in #134 (comment) you fixed #134 issue.

But I would recommend considering simplifying your code like this

	// Group the output if the user specified a group by option
	// Length is equal to one when empty as it contains an empty string
	if len(GroupOutput) == 1 && GroupOutput[0] != "" {
		// Check reporter type to determine how to print
		if _, ok := c.Reporter.(reporter.JsonReporter); ok {
			return reporter.PrintSingleGroupJson(reportGroup.(map[string][]reporter.Report))
		}
		return reporter.PrintSingleGroupStdout(reportGroup.(map[string][]reporter.Report))
	} else if len(GroupOutput) == 2 {
		if _, ok := c.Reporter.(reporter.JsonReporter); ok {
			return reporter.PrintDoubleGroupJson(reportGroup.(map[string]map[string][]reporter.Report))
		}
		return reporter.PrintDoubleGroupStdout(reportGroup.(map[string]map[string][]reporter.Report))
	} else if len(GroupOutput) == 3 {
		if _, ok := c.Reporter.(reporter.JsonReporter); ok {
			return reporter.PrintTripleGroupJson(reportGroup.(map[string]map[string]map[string][]reporter.Report))
		}
		return reporter.PrintTripleGroupStdout(reportGroup.(map[string]map[string]map[string][]reporter.Report))
	}
	return c.Reporter.Print(reports)

HakanVardarr

I added these refactorings to the PR thanks.

ccoVeille

because you are now returning different type result, you are using interface{} here

It leads you to play with type assertion when calling the reporters.

I would suggest you to write something like this

// printReports prints the reports based on the specified grouping and reporter type.
// It returns any error encountered during the printing process.
func (c CLI) printReports(reports []reporter.Report) error {
	if Quiet {
		return nil
	}

	switch len(GroupOutput) {
	case 1:
		return printGroupSingle(reports)
	case 2:
		return printGroupDouble(reports)
	case 3:
		return printGroupTriple(reports)
	default:
		return nil, fmt.Errorf("Invalid number of group outputs: %d", len(GroupOutput))
	}
}

func (c CLI) printGroupSingle(reports []reporter.Report) error {
	if GroupOutput[0] == "" {
		return c.Reporter.Print(reports)
	}

	reportGroup, err := c.GroupBySingle(reports, GroupOutput[0])
	if err != nil {
		return err
	}

	// Check reporter type to determine how to print
	if _, ok := c.Reporter.(reporter.JsonReporter); ok {
		return reporter.PrintSingleGroupJson(reportGroup)
	}

	return reporter.PrintSingleGroupStdout(reportGroup)
}

func (c CLI) printGroupDouble(reports []reporter.Report) error {
	reportGroup, err := c.GroupByDouble(reports, GroupOutput)
	if err != nil {
		return err
	}

	if _, ok := c.Reporter.(reporter.JsonReporter); ok {
		return reporter.PrintDoubleGroupJson(reportGroup)
	}

	return reporter.PrintDoubleGroupStdout(reportGroup)
}

func (c CLI) printGroupTriple(reports []reporter.Report) error {
	reportGroup, err := c.GroupByTriple(reports, GroupOutput)
	if err != nil {
		return err
	}

	if _, ok := c.Reporter.(reporter.JsonReporter); ok {
		return reporter.PrintTripleGroupJson(reportGroup)
	}
	return reporter.PrintTripleGroupStdout(reportGroup)
}

This is mostly pseudocode made in a web browser, it may need some adjustments

ccoVeille · 2024-03-30T16:29:18Z

I added these refactorings to the PR thanks.

May I ask a Co-Authored-By tag in your commit message then ? 😆

https://docs.github.com/articles/creating-a-commit-with-multiple-authors#:~:text=a%20remote%20repository.%22-,Creating%20co%2Dauthored%20commits%20on%20GitHub,address%20for%20each%20co%2Dauthor.

Mine would be something like

Co-authored-by: ccoVeille <[email protected]>

Co-authored-by: ccoVeille <[email protected]>

HakanVardarr · 2024-03-30T17:59:00Z

I added these refactorings to the PR thanks.

May I ask a Co-Authored-By tag in your commit message then ? 😆

https://docs.github.com/articles/creating-a-commit-with-multiple-authors#:~:text=a%20remote%20repository.%22-,Creating%20co%2Dauthored%20commits%20on%20GitHub,address%20for%20each%20co%2Dauthor.

Mine would be something like
Co-authored-by: ccoVeille <[email protected]>

Is this right? First time doing this. :)

ccoVeille · 2024-03-30T18:07:39Z

I added these refactorings to the PR thanks.

May I ask a Co-Authored-By tag in your commit message then ? 😆
https://docs.github.com/articles/creating-a-commit-with-multiple-authors#:~:text=a%20remote%20repository.%22-,Creating%20co%2Dauthored%20commits%20on%20GitHub,address%20for%20each%20co%2Dauthor.
Mine would be something like
Co-authored-by: ccoVeille <[email protected]>
Is this right? First time doing this. :)

I think you should have a new line between the commit message and the Co-authored-by:

ccoVeille · 2024-03-30T18:08:01Z

because you are now returning different type result, you are using interface{} here

It leads you to play with type assertion when calling the reporters.

I would suggest you to write something like this

// printReports prints the reports based on the specified grouping and reporter type.
// It returns any error encountered during the printing process.
func (c CLI) printReports(reports []reporter.Report) error {
	if Quiet {
		return nil
	}

	switch len(GroupOutput) {
	case 1:
		return printGroupSingle(reports)
	case 2:
		return printGroupDouble(reports)
	case 3:
		return printGroupTriple(reports)
	default:
		return nil, fmt.Errorf("Invalid number of group outputs: %d", len(GroupOutput))
	}
}

func (c CLI) printGroupSingle(reports []reporter.Report) error {
	if GroupOutput[0] == "" {
		return c.Reporter.Print(reports)
	}

	reportGroup, err := c.GroupBySingle(reports, GroupOutput[0])
	if err != nil {
		return err
	}

	// Check reporter type to determine how to print
	if _, ok := c.Reporter.(reporter.JsonReporter); ok {
		return reporter.PrintSingleGroupJson(reportGroup)
	}

	return reporter.PrintSingleGroupStdout(reportGroup)
}

func (c CLI) printGroupDouble(reports []reporter.Report) error {
	reportGroup, err := c.GroupByDouble(reports, GroupOutput)
	if err != nil {
		return err
	}

	if _, ok := c.Reporter.(reporter.JsonReporter); ok {
		return reporter.PrintDoubleGroupJson(reportGroup)
	}

	return reporter.PrintDoubleGroupStdout(reportGroup)
}

func (c CLI) printGroupTriple(reports []reporter.Report) error {
	reportGroup, err := c.GroupByTriple(reports, GroupOutput)
	if err != nil {
		return err
	}

	if _, ok := c.Reporter.(reporter.JsonReporter); ok {
		return reporter.PrintTripleGroupJson(reportGroup)
	}
	return reporter.PrintTripleGroupStdout(reportGroup)
}

This is mostly pseudocode made in a web browser, it may need some adjustments

did you see this @HakanVardarr BTW? what are your thoughts about it ?

HakanVardarr · 2024-03-30T18:26:06Z

because you are now returning different type result, you are using interface{} here
It leads you to play with type assertion when calling the reporters.
I would suggest you to write something like this

// printReports prints the reports based on the specified grouping and reporter type.
// It returns any error encountered during the printing process.
func (c CLI) printReports(reports []reporter.Report) error {
	if Quiet {
		return nil
	}

	switch len(GroupOutput) {
	case 1:
		return printGroupSingle(reports)
	case 2:
		return printGroupDouble(reports)
	case 3:
		return printGroupTriple(reports)
	default:
		return nil, fmt.Errorf("Invalid number of group outputs: %d", len(GroupOutput))
	}
}

func (c CLI) printGroupSingle(reports []reporter.Report) error {
	if GroupOutput[0] == "" {
		return c.Reporter.Print(reports)
	}

	reportGroup, err := c.GroupBySingle(reports, GroupOutput[0])
	if err != nil {
		return err
	}

	// Check reporter type to determine how to print
	if _, ok := c.Reporter.(reporter.JsonReporter); ok {
		return reporter.PrintSingleGroupJson(reportGroup)
	}

	return reporter.PrintSingleGroupStdout(reportGroup)
}

func (c CLI) printGroupDouble(reports []reporter.Report) error {
	reportGroup, err := c.GroupByDouble(reports, GroupOutput)
	if err != nil {
		return err
	}

	if _, ok := c.Reporter.(reporter.JsonReporter); ok {
		return reporter.PrintDoubleGroupJson(reportGroup)
	}

	return reporter.PrintDoubleGroupStdout(reportGroup)
}

func (c CLI) printGroupTriple(reports []reporter.Report) error {
	reportGroup, err := c.GroupByTriple(reports, GroupOutput)
	if err != nil {
		return err
	}

	if _, ok := c.Reporter.(reporter.JsonReporter); ok {
		return reporter.PrintTripleGroupJson(reportGroup)
	}
	return reporter.PrintTripleGroupStdout(reportGroup)
}

This is mostly pseudocode made in a web browser, it may need some adjustments

did you see this @HakanVardarr BTW? what are your thoughts about it ?

Oh I didn't see these, I'm looking right now

HakanVardarr · 2024-03-30T18:27:33Z

I think that looks good. I'll add it and co-auther you.

> > Co-authored-by: ccoVeille <[email protected]>

HakanVardarr · 2024-03-30T18:33:14Z

I added the co-authered-by part @ccoVeille is it right?

Co-authored-by: ccoVeille <[email protected]>

ccoVeille · 2024-03-30T19:25:07Z

It's ok 305ebba

You can see here our names in

You can also see it here

The right pattern to use is:

Commit message

Co-authored-by: ccoVeille <[email protected]>

And if you have to add a description it would be

Commit message

Description line1
Description line2
(...)

Co-authored-by: ccoVeille <[email protected]>

pkg/finder/finder_test.go

ccoVeille · 2024-03-30T19:35:34Z

Thanks for following my suggestion. I think we are good.

The code is now clearer and easier to maintain.

ccoVeille · 2024-03-30T19:37:04Z

You just have to check if all tests are good.

I'm a bit worried that your version will report nothing in json mode if quiet is enabled.

So maybe, you will have to remove the if quiet and return nil in each print single/double/triple when quiet is true and it's not about dumping json

HakanVardarr · 2024-03-30T20:34:13Z

You just have to check if all tests are good.

I'm a bit worried that your version will report nothing in json mode if quiet is enabled.

So maybe, you will have to remove the if quiet and return nil in each print single/double/triple when quiet is true and it's not about dumping json

I think this commit solves the issue.

kehoecj

Thank you for the PR @HakanVardarr ! Looks like some good improvements. Can you take a look at the failing unit tests?

HakanVardarr · 2024-04-01T16:30:19Z

I hope they are right I got midterm tomorrow I don't have much time to debug :) @kehoecj

HakanVardarr · 2024-04-01T16:31:56Z

Unit tests are passing but coverage is not. :(

kehoecj · 2024-04-01T16:32:26Z

I hope they are right I got midterm tomorrow I don't have much time to debug :) @kehoecj

No rush! Midterm is more important 😀

HakanVardarr · 2024-04-01T16:34:03Z

I hope they are right I got midterm tomorrow I don't have much time to debug :) @kehoecj

No rush! Midterm is more important 😀

I handled the unit tests but coverage is stuck at 94.9% 😄

ccoVeille · 2024-04-01T16:53:48Z

This may help you

go test -coverprofile=cover.out ./... ; go tool cover -html=cover.out -o cover.html

You will see you can improve coverage in

      if *reportTypePtr == "junit" && *groupOutputPtr != "" {
                fmt.Println("Wrong parameter value for reporter, groupby is not supported for JUnit reports")
                flag.Usage()
                return validatorConfig{}, errors.New("Wrong parameter value for reporter, groupby is not supported for JUnit reports")
        }

and

                        if _, ok := seenValues[groupBy]; ok {
                                fmt.Println("Wrong parameter value for groupby, duplicate values are not allowed")
                                flag.Usage()
                                return validatorConfig{}, errors.New("Wrong parameter value for groupby, duplicate values are not allowed")
                        }

Co-authored-by: ccoVeille <[email protected]>

HakanVardarr · 2024-04-01T17:32:07Z

This may help you

go test -coverprofile=cover.out ./... ; go tool cover -html=cover.out -o cover.html

You will see you can improve coverage in

      if *reportTypePtr == "junit" && *groupOutputPtr != "" {
                fmt.Println("Wrong parameter value for reporter, groupby is not supported for JUnit reports")
                flag.Usage()
                return validatorConfig{}, errors.New("Wrong parameter value for reporter, groupby is not supported for JUnit reports")
        }

and

                        if _, ok := seenValues[groupBy]; ok {
                                fmt.Println("Wrong parameter value for groupby, duplicate values are not allowed")
                                flag.Usage()
                                return validatorConfig{}, errors.New("Wrong parameter value for groupby, duplicate values are not allowed")
                        }

Yep they worked perfect. Thanks, I Co-Authered you to the commit.

ccoVeille · 2024-04-01T19:54:31Z

I would say the number of commits could be reduced by grouping them with an interactive rebase. But I will let maintainers tell what they want.

HakanVardarr · 2024-04-02T05:37:39Z

I would say the number of commits could be reduced by grouping them with an interactive rebase. But I will let maintainers tell what they want.

I don't know much about git stuffs. I am new to this that is why commit number is high.

ccoVeille · 2024-04-02T06:20:50Z

I would say the number of commits could be reduced by grouping them with an interactive rebase. But I will let maintainers tell what they want.

I don't know much about git stuffs. I am new to this that is why commit number is high.

It's OK. Let's wait for the maintainers point of view. Have fun with your midterms. That's more important to worry about.

HakanVardarr · 2024-04-02T11:38:48Z

I would say the number of commits could be reduced by grouping them with an interactive rebase. But I will let maintainers tell what they want.

I don't know much about git stuffs. I am new to this that is why commit number is high.

It's OK. Let's wait for the maintainers point of view. Have fun with your midterms. That's more important to worry about.

I'll try to have fun. 😅

kehoecj

LGTM - thanks @HakanVardarr fixing the report card issues and general cleanup

ccoVeille · 2024-04-10T16:11:39Z

Great @HakanVardarr, I can rebase mine now.

I hope you enjoyed my help on your PR.

ccoVeille · 2024-04-10T18:32:57Z

@kehoecj I appreciate you squashed all the changes: 9006fce

I was unsure the Co-Authored-By would be kept, but I'm happy to see it worked

HakanVardarr · 2024-04-10T21:28:13Z

Great @HakanVardarr, I can rebase mine now.

I hope you enjoyed my help on your PR.

I enjoyed your help. Thanks <3

@ccoVeille

* Fix ineffassign * Fix ineffassign * Fix misspell * Fix gocyclo * Return error * @ccoVeille s suggestion * Returning error Co-authored-by: ccoVeille <[email protected]> * Removed interface{} to solve type problems. > > Co-authored-by: ccoVeille <[email protected]> * Comment Co-authored-by: ccoVeille <[email protected]> * Fix file output on quiet flag * Delete test/result.json * Changed 1 test * Test fix * remove test.txt * Coverage test Co-authored-by: ccoVeille <[email protected]> --------- Co-authored-by: ccoVeille <[email protected]>

@ccoVeille

* Fix ineffassign * Fix ineffassign * Fix misspell * Fix gocyclo * Return error * @ccoVeille s suggestion * Returning error Co-authored-by: ccoVeille <[email protected]> * Removed interface{} to solve type problems. > > Co-authored-by: ccoVeille <[email protected]> * Comment Co-authored-by: ccoVeille <[email protected]> * Fix file output on quiet flag * Delete test/result.json * Changed 1 test * Test fix * remove test.txt * Coverage test Co-authored-by: ccoVeille <[email protected]> --------- Co-authored-by: ccoVeille <[email protected]>

HakanVardarr added 4 commits March 30, 2024 09:10

Fix ineffassign

f12be3b

Fix ineffassign

4415568

Fix misspell

ce003a3

Fix gocyclo

e9d7f48

HakanVardarr mentioned this pull request Mar 30, 2024

None of the error returned by reporters are checked #134

Closed

ccoVeille mentioned this pull request Mar 30, 2024

code review #132

Merged

Return error

b8b0540

ccoVeille suggested changes Mar 30, 2024

View reviewed changes

@ccoVeille s suggestion

c6a51b7

HakanVardarr commented Mar 30, 2024

View reviewed changes

ccoVeille suggested changes Mar 30, 2024

View reviewed changes

Returning error

305ebba

Co-authored-by: ccoVeille <[email protected]>

Removed interface{} to solve type problems.

51ea14d

> > Co-authored-by: ccoVeille <[email protected]>

Comment

3d972d5

Co-authored-by: ccoVeille <[email protected]>

ccoVeille reviewed Mar 30, 2024

View reviewed changes

pkg/finder/finder_test.go Show resolved Hide resolved

Fix file output on quiet flag

621af20

Delete test/result.json

8276f8b

kehoecj added OSS Community Contribution Contributions from the OSS Community waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers labels Apr 1, 2024

kehoecj self-requested a review April 1, 2024 14:04

kehoecj reviewed Apr 1, 2024

View reviewed changes

kehoecj added pr-action-requested PR is awaiting feedback from the submitting developer and removed waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers labels Apr 1, 2024

HakanVardarr added 2 commits April 1, 2024 18:14

Changed 1 test

3cf12fe

Test fix

4d714c5

remove test.txt

fdf2673

Coverage test

6397575

Co-authored-by: ccoVeille <[email protected]>

kehoecj added waiting-on-maintainer-review PR is waiting to be reviewed and functionally tested by the maintainers and removed pr-action-requested PR is awaiting feedback from the submitting developer labels Apr 1, 2024

ccoVeille mentioned this pull request Apr 8, 2024

Fix Go Report Card Issues #130

Closed

kehoecj approved these changes Apr 10, 2024

View reviewed changes

kehoecj merged commit 9006fce into Boeing:main Apr 10, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Go Report Card Issues #131

Fix Go Report Card Issues #131

HakanVardarr commented Mar 30, 2024

ccoVeille left a comment

HakanVardarr left a comment

ccoVeille left a comment

ccoVeille commented Mar 30, 2024 •

edited

Loading

HakanVardarr commented Mar 30, 2024

ccoVeille commented Mar 30, 2024

ccoVeille commented Mar 30, 2024

HakanVardarr commented Mar 30, 2024

HakanVardarr commented Mar 30, 2024

HakanVardarr commented Mar 30, 2024

ccoVeille commented Mar 30, 2024

ccoVeille commented Mar 30, 2024

ccoVeille commented Mar 30, 2024

HakanVardarr commented Mar 30, 2024

kehoecj left a comment

HakanVardarr commented Apr 1, 2024

HakanVardarr commented Apr 1, 2024

kehoecj commented Apr 1, 2024

HakanVardarr commented Apr 1, 2024

ccoVeille commented Apr 1, 2024

HakanVardarr commented Apr 1, 2024

ccoVeille commented Apr 1, 2024

HakanVardarr commented Apr 2, 2024

ccoVeille commented Apr 2, 2024

HakanVardarr commented Apr 2, 2024

kehoecj left a comment

ccoVeille commented Apr 10, 2024

ccoVeille commented Apr 10, 2024

HakanVardarr commented Apr 10, 2024

Fix Go Report Card Issues #131

Fix Go Report Card Issues #131

Conversation

HakanVardarr commented Mar 30, 2024

ccoVeille left a comment

Choose a reason for hiding this comment

HakanVardarr left a comment

Choose a reason for hiding this comment

ccoVeille left a comment

Choose a reason for hiding this comment

ccoVeille commented Mar 30, 2024 • edited Loading

HakanVardarr commented Mar 30, 2024

ccoVeille commented Mar 30, 2024

ccoVeille commented Mar 30, 2024

HakanVardarr commented Mar 30, 2024

HakanVardarr commented Mar 30, 2024

HakanVardarr commented Mar 30, 2024

ccoVeille commented Mar 30, 2024

ccoVeille commented Mar 30, 2024

ccoVeille commented Mar 30, 2024

HakanVardarr commented Mar 30, 2024

kehoecj left a comment

Choose a reason for hiding this comment

HakanVardarr commented Apr 1, 2024

HakanVardarr commented Apr 1, 2024

kehoecj commented Apr 1, 2024

HakanVardarr commented Apr 1, 2024

ccoVeille commented Apr 1, 2024

HakanVardarr commented Apr 1, 2024

ccoVeille commented Apr 1, 2024

HakanVardarr commented Apr 2, 2024

ccoVeille commented Apr 2, 2024

HakanVardarr commented Apr 2, 2024

kehoecj left a comment

Choose a reason for hiding this comment

ccoVeille commented Apr 10, 2024

ccoVeille commented Apr 10, 2024

HakanVardarr commented Apr 10, 2024

ccoVeille commented Mar 30, 2024 •

edited

Loading