Skip to content

Conversation

@mwbrooks
Copy link
Member

@mwbrooks mwbrooks commented May 1, 2025

Summary

This pull request enables the linter staticcheck rule ST1005: 'Incorrectly formatted error string'.

In general, it's enforces the following:

  • Error strings must start with a lowercase letter, unless a proper name or initialism
    • Unfortunately, I couldn't figure out how to make "Slack" a recognized noun
  • Error strings cannot end with punctuation (e.g. periods .)

Requirements

@mwbrooks mwbrooks added code health M-T: Test improvements and anything that improves code health semver:patch Use on pull requests to describe the release version increment labels May 1, 2025
@mwbrooks mwbrooks added this to the Next Release milestone May 1, 2025
@mwbrooks mwbrooks self-assigned this May 1, 2025
@mwbrooks mwbrooks requested a review from a team as a code owner May 1, 2025 20:39
@codecov
Copy link

codecov bot commented May 1, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 63.18%. Comparing base (9437aa2) to head (dfcd379).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/auth/auth.go 0.00% 1 Missing ⚠️
internal/pkg/create/create.go 0.00% 1 Missing ⚠️
internal/update/cli_metadata.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #73      +/-   ##
==========================================
- Coverage   63.19%   63.18%   -0.02%     
==========================================
  Files         210      210              
  Lines       22187    22187              
==========================================
- Hits        14021    14018       -3     
- Misses       7081     7082       +1     
- Partials     1085     1087       +2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mwbrooks mwbrooks modified the milestones: v3.1.0, Next Release May 2, 2025
Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

@mwbrooks I'm hesitating on these changes since I'm not sure that we want to use Errorf in general?

Patterns around slackerror with custom messages seem best to me since it allows us to have coded errors, but these changes might be better for another PR.

Please let me know how you're wanting to proceed! 👾 ✨

@mwbrooks
Copy link
Member Author

mwbrooks commented May 5, 2025

@zimeg My two-cents is that the code is currently using Errorf and the motivation of this PR is to enable the linter rule for error string formatting. I'd prefer to see this PR enable the linter with the current code, then a follow-up PR that switches the errors from Errorf to slackerror.

What do you think? I can handle the follow-up PR to move to slackerror.

Copy link
Member

@zimeg zimeg left a comment

Choose a reason for hiding this comment

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

@mwbrooks Incredible focus once more 👁️‍🗨️

It makes so much sense to make additional changes to errors in a separate PR, meaning this all LGTM! 🤖 ✨

@mwbrooks
Copy link
Member Author

mwbrooks commented May 5, 2025

@zimeg Awesome, I agree, I like the continue momentum forward with smaller, focused PRs rather than trying to jump ahead.

I'll resolve the merge conflict, merge this one in, and then look at updating these Errorf to slackerror. I'll probably want your thoughts on the formatting for the slackerror!

@mwbrooks mwbrooks merged commit eac66e3 into main May 5, 2025
6 checks passed
@mwbrooks mwbrooks deleted the mwbrooks-lint-st1005 branch May 5, 2025 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code health M-T: Test improvements and anything that improves code health semver:patch Use on pull requests to describe the release version increment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants