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

Added staticcheck for basic static code analysis #44

Merged
merged 6 commits into from
Aug 3, 2021
Merged

Added staticcheck for basic static code analysis #44

merged 6 commits into from
Aug 3, 2021

Conversation

oxisto
Copy link
Collaborator

@oxisto oxisto commented Jul 31, 2021

I am basing this off the v4 branch because getting this to work without Go modules is a pain.

Addressed all the issues that static check discovered. Also deprecated several functions that were just used internally.

@oxisto oxisto changed the base branch from main to v4 July 31, 2021 12:54
@oxisto oxisto mentioned this pull request Aug 1, 2021
@oxisto oxisto marked this pull request as ready for review August 1, 2021 14:52
@oxisto oxisto requested a review from mfridman August 1, 2021 14:53
@lggomez lggomez self-requested a review August 2, 2021 22:35
@@ -67,14 +67,14 @@ func start() error {
return showToken()
} else {
flag.Usage()
return fmt.Errorf("None of the required flags are present. What do you want me to do?")
return fmt.Errorf("none of the required flags are present. What do you want me to do?")
Copy link
Member

@lggomez lggomez Aug 2, 2021

Choose a reason for hiding this comment

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

Can we consider this a non-breaking change? personally I fully agree with correcting this style but this will break user tests that perform assertions against the explicit error values. Considering this goes for v4, we can consider it a non-issue but I would think twice if we ever need to backport to v3

Copy link
Collaborator Author

@oxisto oxisto Aug 3, 2021

Choose a reason for hiding this comment

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

Since it is in the actual jwt application, I would say that this is ok. For the other error changes I would assume that people are trying to compare against the error variables and not the content. But anyway, yes, I would keep this for v4 only, which promises compatibility but considering it to be a major version bump, I would argue this has enough leeway to fix issues like this.

Copy link
Member

@lggomez lggomez left a comment

Choose a reason for hiding this comment

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

LGTM (the above comment can be a separate thread)

@oxisto oxisto merged commit 9131873 into v4 Aug 3, 2021
@oxisto oxisto deleted the staticcheck branch August 3, 2021 13:44
oxisto added a commit that referenced this pull request Aug 3, 2021
Additionally, added `staticcheck` for basic static code analysis (#44)

Co-authored-by: Christian Banse <[email protected]>
@oxisto oxisto mentioned this pull request Aug 3, 2021
3 tasks
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