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 some linting warnings (plus future use of staticcheck?) #226

Merged
merged 5 commits into from
Aug 18, 2023

Conversation

c9845
Copy link

@c9845 c9845 commented Jul 26, 2023

  • Comment out unused code.
  • Fix incorrect JSON struct tag (missing opening double-quote).
  • Remove unneeded type inside slice of type ("redundant type from array, slice, or map composite literal" warning).

There are a lot of other warnings shown by staticcheck that were not fixed (staticcheck isn't in use for this repo). Most of these warnings are for the checks listed below. Fixing these is a rather big change, just due to volume of lines changed. Whether or not we want to implement staticcheck is the question.

  • ST1003 (using Api instead of API in struct field name).
  • ST1021 (not starting the documentation of an exported type with it's name.
  • ST1020 (not starting the documentation of an exported method or func with the method's name).

Changing field names per warning ST1003 is a breaking change, so is probably best to be avoided (staticcheck.conf would be set to checks="all", "-ST1003"]). Fixes for the other warnings are no issue since these are just a documentation change.

Examples to be fixed...

  • abandoned_checkout.go:25:1: comment on exported type AbandonedCheckoutsResource should be of the form "AbandonedCheckoutsResource ..." (with optional leading article) (ST1021)
  • abandoned_checkout.go:85:1: comment on exported method List should be of the form "List ..." (ST1020)
  • blog.go:49:1: comment on exported type BlogResource should be of the form "BlogResource ..." (with optional leading article) (ST1021)
  • carrier.go:75:1: comment on exported type ShippingRateAddress should be of the form "ShippingRateAddress ..." (with optional leading article) (ST1021)
  • carrier.go: 100:1: comment on exported type ShippingRateResponse should be of the form "ShippingRateResponse ..." (with optional leading article) (ST1021)
  • collect.go:39:1: comment on exported type CollectResource should be of the form "CollectResource ..." (with optional leading article) (ST1021)
  • collect.go:44:1: comment on exported type CollectsResource should be of the form "CollectsResource ..." (with optional leading article) (ST1021)
  • collection.go:39:1: comment on exported type CollectionResource should be of the form "CollectionResource ..." (with optional leading article) (ST1021)
  • collection.go:52:1: comment on exported method ListProducts should be of the form "ListProducts ..." (ST1020)

c9845 (AA) added 2 commits July 26, 2023 17:47
- Comment out unused code.
- Fix incorrect JSON struct tag (missing opening double-quote).
- Remove unneeded type inside slice of type ("redundant type from array, slice, or map composite literal" warning).

There are a lot of other warnings shown by staticcheck that were not fixed yet.  These all the warnings are for the checks listed below.  Fixing these is a rather big change.  Changing field names per ST1003 is a breaking change, so is probably best to be avoided.  Fixes for the other warnings is no issue since it is just a documentation change.

- ST1003 (using Api instead of API in struct field name),
- ST1021 (not starting the documentation of an exported type with it's name, and
- ST1020 (not starting the documentation of an exported method or func with the method's name).

Examples to be fixed...
abandoned_checkout.go:25:1: comment on exported type AbandonedCheckoutsResource should be of the form "AbandonedCheckoutsResource ..." (with optional leading article) (ST1021)
abandoned_checkout.go:85:1: comment on exported method List should be of the form "List ..." (ST1020)
blog.go:49:1: comment on exported type BlogResource should be of the form "BlogResource ..." (with optional leading article) (ST1021)
carrier.go:75:1: comment on exported type ShippingRateAddress should be of the form "ShippingRateAddress ..." (with optional leading article) (ST1021)
carrier.go:100:1: comment on exported type ShippingRateResponse should be of the form "ShippingRateResponse ..." (with optional leading article) (ST1021)
collect.go:39:1: comment on exported type CollectResource should be of the form "CollectResource ..." (with optional leading article) (ST1021)
collect.go:44:1: comment on exported type CollectsResource should be of the form "CollectsResource ..." (with optional leading article) (ST1021)
collection.go:39:1: comment on exported type CollectionResource should be of the form "CollectionResource ..." (with optional leading article) (ST1021)
collection.go:52:1: comment on exported method ListProducts should be of the form "ListProducts ..." (ST1020)
@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (d54f22b) 100.00% compared to head (7868d1a) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #226   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           46        46           
  Lines         2088      2088           
=========================================
  Hits          2088      2088           
Files Changed Coverage Δ
customer_address.go 100.00% <ø> (ø)
product_listing.go 100.00% <ø> (ø)
goshopify.go 100.00% <100.00%> (ø)

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

@oliver006
Copy link
Collaborator

Thanks for opening this PR - it still seems to be WIP - ping me when you're ready for review or feedback.

@c9845
Copy link
Author

c9845 commented Aug 2, 2023

@oliver006 I am done with this for now.

The questions becomes, do we want to implement staticcheck for the repo. If yes, then this becomes a much larger change.

@oliver006
Copy link
Collaborator

Thanks for clarifying.
Leaving commented out code in the repo is not a good practice, dead code should just be removed unless there's a very good reason.
The rest of the changes look alright.
If you could clean up the commented-out code stuff then I think we should e good to go.

@c9845
Copy link
Author

c9845 commented Aug 7, 2023

@oliver006 Commented out code has been removed.

What are your thoughts on utilizing staticcheck?

@oliver006
Copy link
Collaborator

Thanks @c9845 - this should be good to go.
I don't think there's an urgent need for adding staticcheck to the CI right now. If you submit a PR then I'll review it but I don't have the time right now to add it myself.

Copy link
Collaborator

@oliver006 oliver006 left a comment

Choose a reason for hiding this comment

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

👍

@oliver006 oliver006 merged commit d5d643a into bold-commerce:master Aug 18, 2023
5 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.

2 participants