Fix Go Report Card Issues

[HakanVardarr]

I fixed all of the issues listed on goreportcard.

[ccoVeille]

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 <3875889+ccoVeille@users.noreply.github.com>

[HakanVardarr]

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 <3875889+ccoVeille@users.noreply.github.com>

Is this right? First time doing this. :)

[ccoVeille]

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 <3875889+ccoVeille@users.noreply.github.com>

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]

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]

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]

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

[HakanVardarr]

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

[ccoVeille]

It's ok 305ebbabb5c76d9aafe50cc4df1efb970a8591bc

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 <3875889+ccoVeille@users.noreply.github.com>

And if you have to add a description it would be

Commit message

Description line1
Description line2
(...)

Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>

[ccoVeille]

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

The code is now clearer and easier to maintain.

[ccoVeille]

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]

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.

[HakanVardarr]

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

[HakanVardarr]

Unit tests are passing but coverage is not. :(

[kehoecj]

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]

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]

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")
                        }

[HakanVardarr]

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]

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]

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]

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]

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. 😅

[ccoVeille]

Great @HakanVardarr, I can rebase mine now.

I hope you enjoyed my help on your PR.

[ccoVeille]

@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]

Great @HakanVardarr, I can rebase mine now.

I hope you enjoyed my help on your PR.

I enjoyed your help. Thanks <3