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

feat: fix imports #283

Closed
wants to merge 3 commits into from
Closed

feat: fix imports #283

wants to merge 3 commits into from

Conversation

h5law
Copy link
Contributor

@h5law h5law commented Dec 17, 2023

Summary

Human Summary

Add bash script to add goimports-reviser import grouping excluding files where scaffolding comments are in the import area. This may need to be improved upon in fututre but its a start at least.

I also include all the changes it makes

NOTE: Go Imports Reviser removes those ignite scaffold comments from the imports block and I dont have the bash foo to solve this rn and GPT didnt either. So I manually fixed.

AI Summary

Summary generated by Reviewpad on 17 Dec 23 02:19 UTC

This pull request includes the following changes:

  1. The file key_gateway.go in the x/gateway/types package has undergone changes in the import section.
  2. The file client_integration_test.go has changes related to package imports.
  3. The file errors.go in the pkg/appgateserver/config package has undergone import and variable changes.
  4. The file x/tokenomics/keeper/query_params_test.go includes import additions and removals.
  5. The file goimports-revisor.sh is a new script that processes Go files with specific flags and exclusions.
  6. The file params_test.go has changes in the import statements.
  7. The file tx.go in the x/gateway/client/cli directory includes import additions and removals.
  8. The file session_steps_test.go includes changes to import statements and code.
  9. The file urls.go in the pkg/sdk package has import statement modifications.
  10. The file replay_client_example_test.go has changes in import statements.
  11. The file genesis.go in the x/session/types package has import and comment changes.
  12. The file unstake_application.go has an unused import removal.
  13. The file tokenomics/types/genesis.go has import block removal and constant addition.
  14. The file helpers_test.go has import additions, removals, and repositioning.
  15. The file tx.go in the x/tokenomics/client/cli package includes import statement modifications.
  16. The file delegate_to_gateway.go has import reordering and movement.
  17. The file genesis_test.go in the x/tokenomics/types package includes import order changes.
  18. The file session_steps_test.go has code and import changes.
  19. The file urls.go has a blank line addition.
  20. The file testutil/testerrors/require.go has import statement changes and error handling modifications.
  21. The file genesis_test.go in the x/tokenomics/types package has import order changes and an added import.
  22. The file relay.go has an import change.
  23. The file errors.go in the pkg/appgateserver package has import changes and variable definition.
  24. The file key_supplier.go in the types package has import updates.
  25. The file message_delegate_to_gateway_test.go in the types package has import statement changes.

Please review the complete diff for more details on each file.

Issue

Related #270

Type of change

Select one or more:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

  • Run all unit tests: make go_develop_and_test
  • Run E2E tests locally: make test_e2e
  • Run E2E tests on DevNet: Add the devnet-test-e2e label to the PR. This is VERY expensive, only do it after all the reviews are complete.

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have performed a self-review of my own code
  • I have commented my code, updated documentation and left TODOs throughout the codebase

@h5law h5law self-assigned this Dec 17, 2023
@h5law h5law added the code health Cleans up some code label Dec 17, 2023
@h5law h5law added this to the Shannon Quality of Life milestone Dec 17, 2023
@h5law
Copy link
Contributor Author

h5law commented Dec 17, 2023

go lint fails bc goimports-reviser removes comments from import blocks and scaffold puts them in there so..,..

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

I haven't done research on the different types of import linters so I don't have a strong opinion on this and interested in hearing @bryanchriswhite's opinion.

  1. @h5law Why this over goimports which is already done?
  2. @bryanchriswhite Wdyt?
  3. If we do go with this approach though (TBD), I'm seeing some outstanding items in this PR:
    1. Update the Makefile to use this importer
    2. Updating the CI to use this importer
    3. Removing ./tools/scripts/goimports because we should only have one import linting script.

@@ -1,8 +1,6 @@
package types

import (
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what to do, but just calling it out: I'm a bit woried about this one. It could break future ignite scaffold calls.

Not a hard blocker if we choose to go with this approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we can revert these the issue with ignite stuff is that linters dont allow comments in the import block

@bryanchriswhite
Copy link
Contributor

bryanchriswhite commented Dec 21, 2023

@Olshansk @h5law

TL;DR, I think we should be reluctant to introduce redundant tooling without clear and substantial benefits to using the new thing over the old thing. golangci-lint bundles GCI, which supports the import grouping we want.

See #253 (comment) and #253 (comment).
image

image

@Olshansk
Copy link
Member

Olshansk commented Jan 3, 2024

Could/should we close this one out?

@bryanchriswhite bryanchriswhite linked an issue Jan 4, 2024 that may be closed by this pull request
4 tasks
@h5law
Copy link
Contributor Author

h5law commented Jan 5, 2024

@Olshansk going to close this one - its outdated and better to add tooling instead of a simple fix like this.

@h5law h5law closed this Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Cleans up some code
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[Code Health] Standardize & enforce golang import linting
3 participants