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

✨ [migration to score] 1: create errors and new functions #712

Merged
merged 8 commits into from
Jul 20, 2021

Conversation

laurentsimon
Copy link
Contributor

@laurentsimon laurentsimon commented Jul 19, 2021

  • Please check if the PR fulfills these requirements
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    feature

  • What is the current behavior? (You can also link to an open issue here)
    we use fail/pass

  • What is the new behavior (if this is a feature change)?
    we define new function to move to score-based

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    no

  • Other information:

  1. create errors, both internal and public
  2. create new result-creating functions in checker
  3. create doc about errors and how to write checks
  4. add a --v2 CLI argument to enable us to check the new version, and a function to display the new result.

@laurentsimon laurentsimon changed the title ✨ migration to score [create errors and new functions] ✨ [migration to score][1]: create errors and new functions Jul 19, 2021
@laurentsimon laurentsimon changed the title ✨ [migration to score][1]: create errors and new functions ✨ [migration to score] 1: create errors and new functions Jul 19, 2021
pkg/scorecard.go Outdated
@@ -22,14 +23,13 @@ import (
"time"

"github.com/google/go-github/v32/github"
"github.com/shurcooL/githubv4"
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert changes in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems the dependency is not needed. I did not purposely remove it, but my editor figured it was not needed and removed it when I saved the file. No harm in removing it then?

Copy link
Contributor

@oliverchang oliverchang left a comment

Choose a reason for hiding this comment

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

nice!

is this split out from #650. Do we still need to review that one? (Sorry for the delay)

checker/check_result.go Show resolved Hide resolved
errors/errors.md Outdated Show resolved Hide resolved
checker/check_result.go Outdated Show resolved Hide resolved
// UPGRADEv2: New structure. Omitting unchanged Name field
// for simplicity.
Version int `json:"-"` // Default value of 0 indicates old structure.
Error2 error `json:"-"` // Runtime error indicate a filure to run the check.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need an Error2, or can we just reuse the Error field?

pkg/scorecard_result.go Outdated Show resolved Hide resolved
checks/write.md Outdated Show resolved Hide resolved

//nolint
var (
ErrInternalInvalidDockerFile = errors.New("invalid Dockerfile")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these are internal errors, these can simply be in checks package and not be exported?

Copy link
Contributor Author

@laurentsimon laurentsimon Jul 20, 2021

Choose a reason for hiding this comment

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

good point. I initially put them all in errors/ so they are all in one place, but I think we can move those to checks/ unless we think they may be shared with other parts of the code, e.g. pkg/.
Will move internal.go to checks/errors.go
Please confirm that's OK.

errors/public.go Outdated Show resolved Hide resolved
errors/public.go Outdated Show resolved Hide resolved
@laurentsimon laurentsimon merged commit ab4bb60 into ossf:main Jul 20, 2021
@laurentsimon
Copy link
Contributor Author

I've addressed all comments. Merging.

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.

3 participants