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

goal abi method outfile checking #3204

Merged
merged 3 commits into from
Nov 18, 2021

Conversation

barnjamin
Copy link
Contributor

Check if the outFilename argument is passed and write to the path specified as other commands

ahangsu
ahangsu previously approved these changes Nov 10, 2021
Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2021

Codecov Report

Merging #3204 (bf1d28a) into master (d3bbe62) will decrease coverage by 0.01%.
The diff coverage is 30.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3204      +/-   ##
==========================================
- Coverage   47.12%   47.11%   -0.02%     
==========================================
  Files         366      366              
  Lines       58675    58681       +6     
==========================================
- Hits        27653    27645       -8     
- Misses      27804    27816      +12     
- Partials     3218     3220       +2     
Impacted Files Coverage Δ
cmd/goal/application.go 12.77% <30.76%> (-0.45%) ⬇️
ledger/roundlru.go 90.56% <0.00%> (-5.67%) ⬇️
ledger/blockqueue.go 81.03% <0.00%> (-4.03%) ⬇️
catchup/service.go 69.07% <0.00%> (-0.75%) ⬇️
network/wsPeer.go 68.05% <0.00%> (-0.56%) ⬇️
data/transactions/verify/txn.go 44.29% <0.00%> (ø)
network/requestTracker.go 71.12% <0.00%> (+0.86%) ⬆️
ledger/acctupdates.go 65.96% <0.00%> (+0.95%) ⬆️
data/abi/abi_type.go 93.03% <0.00%> (+0.99%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3bbe62...bf1d28a. Read the comment docs.

Comment on lines -183 to -184
methodAppCmd.Flags().MarkHidden("app-input") // nolint:errcheck
methodAppCmd.Flags().MarkHidden("i") // nolint:errcheck
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these and why did they go away?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suppose we want to support foreignAssets/foreignApp/etc., we can pass that in cmd line and I suppose they can also be passed from a file, right?
This was left from previous #3088 about if keeping argument for foreignApp etc., and I think the option should be available and kept

Copy link
Member

Choose a reason for hiding this comment

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

The only things that went away are:

methodAppCmd.Flags().MarkHidden("app-input") // nolint:errcheck
methodAppCmd.Flags().MarkHidden("i")         // nolint:errcheck

They are both flags which specify a JSON input file which can contain foreign references (and app args, but we already handle these properly). Since we want to allow specifying additional foreign references, I think it makes sense to keep this flag unhidden.

@ahangsu ahangsu closed this Nov 10, 2021
@ahangsu ahangsu reopened this Nov 10, 2021
Copy link
Member

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Mostly good, I just have one suggestion

Comment on lines -183 to -184
methodAppCmd.Flags().MarkHidden("app-input") // nolint:errcheck
methodAppCmd.Flags().MarkHidden("i") // nolint:errcheck
Copy link
Member

Choose a reason for hiding this comment

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

The only things that went away are:

methodAppCmd.Flags().MarkHidden("app-input") // nolint:errcheck
methodAppCmd.Flags().MarkHidden("i")         // nolint:errcheck

They are both flags which specify a JSON input file which can contain foreign references (and app args, but we already handle these properly). Since we want to allow specifying additional foreign references, I think it makes sense to keep this flag unhidden.

cmd/goal/application.go Show resolved Hide resolved
@algojohnlee algojohnlee merged commit 83ca8ec into algorand:master Nov 18, 2021
@egieseke egieseke mentioned this pull request Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants