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 make lint #596

Closed
wants to merge 2 commits into from
Closed

Conversation

estenssoros
Copy link
Contributor

Description

I was attempting to add a new feature and ran into a significant amount of errors running make lint. I attempted to fix them here, but wasn't sure how you wanted to structure the constants and/or if I should split this into multiple PRs. There are some changes to non-test files which might introduce breaking changes so maybe this is just a bad idea overall. Let me know what you think.

Testing plan

External links

Output from tests

Including output from tests may require access to a TFE instance. Ignore this section if you have no environment to test against.

TFE_ADDRESS="https://example" TFE_TOKEN="example" go test ./... -v -run TestFunctionsAffectedByChange
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/go-tfe	0.002s [no tests to run]
?   	github.com/hashicorp/go-tfe/examples/organizations	[no test files]
?   	github.com/hashicorp/go-tfe/examples/workspaces	[no test files]
?   	github.com/hashicorp/go-tfe/mocks	[no test files]

...

@estenssoros estenssoros requested a review from a team as a code owner November 30, 2022 16:25
@hashicorp-cla
Copy link

hashicorp-cla commented Nov 30, 2022

CLA assistant check
All committers have signed the CLA.

@brandonc
Copy link
Collaborator

@estenssoros Hello! Thanks so much for drawing my attention to this. Over time, the linter errors can become cluttered because our CI action only checks changed code and it is also not a required check for merge.

Your changes are really appreciated, but I found several problems that I didn't want to make you correct, so I created a new commit and added you as co-author #597

Here's what I found:

  • two of our tests were incorrectly build-tagged so they weren't being considered as callers for the unparam linter. These tests were being skipped by our CI! I had no idea until I started working through the linter issues.
  • We intended not to use the goconst linter for test files, so after excluding those files a lot of changes became unnecessary. I think literals as test descriptions are far nicer than variables.
  • Unfortunately, we can't change "Ascii" to "ASCII" without breaking the API and having to release as 2.0 😬

Thanks again! I'll get this merged when it passes CI and you can rebase your feature onto main after.

@brandonc brandonc closed this Nov 30, 2022
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.

None yet

3 participants