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 multi block argument and add some unit tests #14

Merged
merged 6 commits into from
Aug 8, 2023

Conversation

ipatka
Copy link
Contributor

@ipatka ipatka commented Jul 31, 2023

Multi block commands were not working due to a conflict in clap settings which caused commands like this to fail with a parser error:

cryo txs --blocks 16000000 16000001 16000002

The allow_hyphen_values conflicted with the multiple arguments parsing for block values.

allow_hyphen_values(true),

This PR adds a value terminator which is required to end the block list

This PR changes blocks to expect single input, wrapped in quotes

cryo txs --blocks "16000000 16000001 16000002"

Also added some unit tests while i was looking around

@ipatka
Copy link
Contributor Author

ipatka commented Jul 31, 2023

looks like formatter is failing but not sure how to fix locally, running cargo fmt in root directory

@sslivkoff
Copy link
Member

pr looks good

the allow_hyphen_values conflict with num_args is unfortunate. hyphen args is necessary for using things like --blocks -1000:latest which is a fairly important use case. but needing to use multiple blocks is another important use case

the semicolon syntax is not the end of the world but it's a bit jarring and it will be inconsistent with the usage of cryo's other multi-arg options

I think in a more perfect world the parsing of --blocks would be conditional. something like "if there are multiple args, make it allow_hyphen_values=true, but if there's just a single arg, make it allow_hyphen_values=false". not sure if there's a reasonable way to implement this in clap tho

some options here in order from easiest to hardest

  1. --blocks 16000000 16000001 16000002;
  2. --blocks "16000000 16000001 16000002"
  3. do a multistage parse, try to parse with allow_hyphen_values=true num_args=1, if that fails, try with allow_hyphen_values=false num_args=1..
  4. implement some custom clap args thing

I'm thinking that option 2 would be best because it is most consistent with other cli tools and with cryo's other multi-arg options

@sslivkoff
Copy link
Member

to pass format checks need to use cargo +nightly fmt. this should be added to the README...

@ipatka
Copy link
Contributor Author

ipatka commented Aug 1, 2023

i like option 2 as well, should just involve a minor change to arg parsing, will try it out

@sslivkoff
Copy link
Member

ok yea try it out. I think that would be the best short term solution

longer term we can explore more complex parsing options for making it so the quotations are optional

@ipatka ipatka marked this pull request as draft August 1, 2023 19:06
@ipatka
Copy link
Contributor Author

ipatka commented Aug 1, 2023

Converted to draft while I look into the alternative methods to the terminator

@ipatka ipatka marked this pull request as ready for review August 7, 2023 17:41
@ipatka
Copy link
Contributor Author

ipatka commented Aug 7, 2023

@sslivkoff I go option 2 working, i think it's cleaner than the terminator version. simply need to wrap multi block inputs with double quotes

ex. cryo logs -b "1000 2000"
or more complex
cryo logs -b "15M:+1 1000:1002 -3:1b 2000"

@sslivkoff
Copy link
Member

awesome stuff

@sslivkoff sslivkoff merged commit f417fca into paradigmxyz:main Aug 8, 2023
3 checks passed
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