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

Re-enable linters in CLI #253

Merged
merged 5 commits into from
Feb 28, 2024
Merged

Re-enable linters in CLI #253

merged 5 commits into from
Feb 28, 2024

Conversation

taimoorgit
Copy link
Contributor

Depends on #241

@taimoorgit taimoorgit self-assigned this Feb 27, 2024
@taimoorgit taimoorgit changed the base branch from main to ta/staticcheck-cli February 27, 2024 22:51
@taimoorgit taimoorgit changed the title Re-enable linters Re-enable linters in CLI Feb 27, 2024
Copy link

codecov bot commented Feb 27, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 6.29%. Comparing base (2cc73a6) to head (009c690).
Report is 6 commits behind head on main.

❗ Current head 009c690 differs from pull request most recent head 0fbd44c. Consider uploading reports for the commit 0fbd44c to get more accurate results

Files Patch % Lines
src/common/helpers.go 80.00% 1 Missing ⚠️
src/common/prompts.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main    #253   +/-   ##
=====================================
  Coverage   6.29%   6.29%           
=====================================
  Files          5       5           
  Lines        254     254           
=====================================
  Hits          16      16           
  Misses       238     238           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -9,9 +9,10 @@ import (
"strconv"
"strings"

"github.com/pkg/errors"
Copy link
Contributor

Choose a reason for hiding this comment

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

"github.com/pkg/errors" is a publicly archived project with a release date of 4 years ago.
If we really want the slick error wrapping feature, let's just pull the good stuff out of that repo and add it inside of handleErr().
That also gets us one step closer to a "unified error message" if that's a long term goal

Copy link
Contributor

@davidbloss davidbloss left a comment

Choose a reason for hiding this comment

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

If we want to beef up error messages that's all good, let's just do that without adding an archived dependency

@@ -169,13 +172,6 @@ func init() {
graphqlCmd.Flags().StringArrayP("field", "f", nil, "Add a variable in `key=value` format")
}

func handleErr(msg string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can add cobra.CheckErr() once here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidbloss restored that function and removed all mentions of errors package. Now using fmt.Errorf everywhere to make errors, which is the idiomatic way.

Base automatically changed from ta/staticcheck-cli to main February 28, 2024 14:35
Copy link
Contributor

@davidbloss davidbloss left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

@taimoorgit taimoorgit merged commit e576a8d into main Feb 28, 2024
4 checks passed
@taimoorgit taimoorgit deleted the ta/re-enable-linters branch February 28, 2024 15:04
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.

2 participants