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: add missing tx.go file by default and enable cli if autocli does not exist #3849

Merged
merged 12 commits into from
Dec 20, 2023

Conversation

Pantani
Copy link
Collaborator

@Pantani Pantani commented Dec 19, 2023

Description

Scaffold the x/<module-name>/client/cli/tx.go by default, and enable the GetTxCmd() module command. Now, it is a little bit confusing. We need to uncomment the method to allow the CLI to command the package. If not enabled, the command appears without the args.

@Pantani Pantani self-assigned this Dec 19, 2023
@Pantani Pantani changed the title fix: add missing tx.go file by default and enable cli if autocli not exist fix: add missing tx.go file by default and enable cli if autocli does not exist Dec 19, 2023
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

What do you mean exactly? The only use case you need to uncomment it is for IBC command. Otherwise you should not create it as AutoCLI takes care of it.
A new chain in most cases should not need any cli code.

We should imho uncomment the GetTxCommand instead when scaffolding an IBC command, and let that file be created only for IBC commands.

@Pantani
Copy link
Collaborator Author

Pantani commented Dec 19, 2023

What do you mean exactly? The only use case you need to uncomment it is for IBC command. Otherwise you should not create it as AutoCLI takes care of it. A new chain in most cases should not need any cli code.

We should imho uncomment the GetTxCommand instead when scaffolding an IBC command, and let that file be created only for IBC commands.

If you scaffold a packet:
E.g.: ignite scaffold packet ibcPost title content --ack postID --module blog

and if you try to use the planetd tx ibcPost CLI command for this package, the command shows that it doesn't have arguments, and you can't use the command.

Now, I changed the PR to scaffold the tx.go only if it is an IBC module.

@julienrbrt
Copy link
Member

Oh yes, I understand what is happening. It uses the default AutoCLI config for IBC, which we don't want in that case.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

The change makes sense, but I still don't think we should create the tx.go by default. The previous logic would create it only when it is an IBC module as well. I think only the module.go wiring was missing?

@julienrbrt julienrbrt self-requested a review December 20, 2023 19:09
julienrbrt
julienrbrt previously approved these changes Dec 20, 2023
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

tACK! One nit

@Pantani Pantani enabled auto-merge (squash) December 20, 2023 22:10
Copy link

codecov bot commented Dec 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bbf6931) 24.36% compared to head (311fc6b) 24.37%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3849      +/-   ##
==========================================
+ Coverage   24.36%   24.37%   +0.01%     
==========================================
  Files         294      294              
  Lines       24555    24547       -8     
==========================================
+ Hits         5982     5984       +2     
+ Misses      18036    18027       -9     
+ Partials      537      536       -1     
Files Coverage Δ
ignite/templates/ibc/packet.go 0.00% <ø> (ø)

... and 1 file with indirect coverage changes

@Pantani Pantani merged commit 816a327 into main Dec 20, 2023
43 checks passed
@Pantani Pantani deleted the fix/module-cli-tx-cmd branch December 20, 2023 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants