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

fix: cache/gcs dropped errs #938

Merged
merged 1 commit into from
Aug 3, 2023
Merged

fix: cache/gcs dropped errs #938

merged 1 commit into from
Aug 3, 2023

Conversation

alrs
Copy link
Contributor

@alrs alrs commented Aug 3, 2023

This picks up two dropped err variables in cache/gcs.

@alrs alrs requested review from gdey and ARolek as code owners August 3, 2023 11:22
@coveralls
Copy link

coveralls commented Aug 3, 2023

Pull Request Test Coverage Report for Build 2917c6ff8-PR-938

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 46.906%

Totals Coverage Status
Change from base Build a7bf00420: 0.0%
Covered Lines: 6556
Relevant Lines: 13977

💛 - Coveralls

Copy link
Member

@ARolek ARolek left a comment

Choose a reason for hiding this comment

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

Good catch! Please change the error handling to a return.

cache/gcs/gcs.go Outdated
Comment on lines 50 to 52
if err != nil {
log.Fatal(err)
}
Copy link
Member

Choose a reason for hiding this comment

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

please return the error rather than log fatal.

cache/gcs/gcs.go Outdated
Comment on lines 57 to 59
if err != nil {
log.Fatal(err)
}
Copy link
Member

Choose a reason for hiding this comment

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

please return the error rather than log fatal.

@alrs
Copy link
Contributor Author

alrs commented Aug 3, 2023

Done. I was following the example on

log.Fatal(err)

Should I change that one as well?

@ARolek
Copy link
Member

ARolek commented Aug 3, 2023

Should I change that one as well?

Ooof. Yes please!

@alrs
Copy link
Contributor Author

alrs commented Aug 3, 2023

I've replaced the log.Fatal() with a return, as well.

Copy link
Member

@ARolek ARolek left a comment

Choose a reason for hiding this comment

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

LGTM!

@ARolek ARolek merged commit 7e316eb into go-spatial:master Aug 3, 2023
8 checks passed
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