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

Suggestions for arguments and beyond #1

Closed
wants to merge 1 commit into from

Conversation

mutantcornholio
Copy link

@mutantcornholio mutantcornholio commented Nov 22, 2022

Questions that arose

  1. id field in commands seems redundant: it brings ambiguity between itself and the file name. I'd say let's use file name only.
  2. WDYT: get rid from label, leave only id (force same CLI-friendly names for both CLI and UI) + description?
  3. Do we display arguments for presets where the type is type_one_of with a single argument inside? If we still want to show it, should we automatically mark those as default, as it is the only possible option, and we shouldn't force people to type it all the time?
  4. For bench, runtime argument is a type_many_of? Sure about that? How would it work?
  5. WDYT: default values for args?
  6. WDYT: short options for args? So not only --type would work, but also -t.
  7. WDYT: Pass to scripts not only ARGS, but also REPOSITORY and PRESET
  8. commandStart seems also to be redundant. We're already using folder name for *.cmd.json (it has to be the same, right?), so why would we invent anything for *.sh? Hope that it's not just for sample command. If so, let's add a simple script there, but remove commandStart from all other scripts.
  9. WDYT: default presets, which can be omitted? See "sample"

How does calling the commands look with suggested changes?

bot

bot bench polkadot --type=runtime
bot bench cumulus-starters -t xcm
bot bench cumulus-assets -t xcm -r statemine,westmint,test-utils

bot sample -i "Hello there!"

@mordamax
Copy link
Collaborator

I agree with most of the points, for the long term they make sense to me. + this approach doesn't conflict with anykind of interface (UI,CLI,ChatBot). The only concern from me is the transition/rollout process.

AFAICS We can go ahead and implement the named arguments in 2 ways now

  1. gradually with minimum-to-0 breaking changes to an arguments side
    a) /cmd help + /cmd bench $ xcm kusama-dev pallet_referenda_fellowship, so for people we just get rid of -c -v stuff and provide a documentation, while it will be still possible to run it even with old way.
    b) we build UI and for most of the cases you don't need to use ChatBot experience from GH comments
    c) we migrate the arguments API and not ChatBot will look like this /cmd bench cumulus-starters -t xcm, while on UI the change won't be noticeable
  2. introduce breaking change
    a) /cmd help + /cmd bench cumulus-starters -t xcm
    b) build UI as an addition to ChatBot experience

With 1st for me what's valuable is that it still gives nicer experience but doesn't require you to learn whole new way of how to run it now. While later with UI - some people will have alternative options.
With 2nd, we reduce step 1.c) and implement correct schema straight away. The benefit for us - less work later, however I feel that we will have to take care about learning curve, and handle complaints "command bot doesn't respond to me"

To summarize - i'd go with named arguments, how you suggest, but through 1st, gradual steps, right after the UI as an alternative is provided

@mutantcornholio WDYT ⤴?

Re. "WDYT" questions:

  1. id field in commands seems redundant: it brings ambiguity between itself and the file name. I'd say let's use file name only.
  • 100%
  1. WDYT: get rid from label, leave only id (force same CLI-friendly names for both CLI and UI) + description?
  • yea, could be this way too, since we have description anyway.
  • Re CLI-friendly: IMHO the CLI yet another interface for interaction. UI is another one. The motivation of "label" is to have something called human-friendly, which may include spaces and diff letter-case, so not having labels, doesn't mean to me that it's more CLI friendly, it's more kinda less-human-friendly if we can't name things how we want :D. But as i mentioned - description field should assure that we are still human-friendly, so i think lets remove label
  1. Do we display arguments for presets where the type is type_one_of with a single argument inside? If we still want to show it, should we automatically mark those as default, as it is the only possible option, and we shouldn't force people to type it all the time?
  • In terms of UI - if this is the only one option, so nothing to click, it's pre-selected. With positioned arguments it's required to specify even if there's one argument. With named arguments, as you say, I agree, we shouldn't force to type.
  1. For bench, runtime argument is a type_many_of? Sure about that? How would it work?
  1. WDYT: default values for args?
  • in scope of non-positioned named args - should be good, but as i thought through above - would be great to learn the usage and whether defaulting may actually help. I afraid it could be hard to remember all default values per preset considering the fact that the presets/arguments are be repo-dependent and there could be a lot of them (like for cumulus) to remember which are the default ones. Now logically it makes a lot of sense for sure, but practically we can only guess, this is something for us to figure out.
  1. WDYT: short options for args? So not only --type would work, but also -t.
  • TBH - make sense only if they would be more firmly "standard" and never change. if we 1) have repo-dependent sets of arguments, 2) user-created commands, so they can create their own short commands - i'm not sure if someone will try to remember it. If we think in terms of UI - there's no need. In terms of GH Comments (ChatBot) - how people will run them? there's no history unless you open old PRs and copy from there. So I'm not sure if short-options will be usable, if there's more easy-to-use alternative
  1. WDYT: Pass to scripts not only ARGS, but also REPOSITORY and PRESET
  • if I understand correctly - passing the REPOSITORY makes sense, so you don't infer it from the folder name. However I didn't get full why the shell script need a preset ? May be would be good to discuss via voice
  1. commandStart seems also to be redundant. We're already using folder name for *.cmd.json (it has to be the same, right?), so why would we invent anything for *.sh? Hope that it's not just for sample command. If so, let's add a simple script there, but remove commandStart from all other scripts.
  • 100%
  1. WDYT: default presets, which can be omitted? See "sample"
  • cool idea, but I feel same as for answers No.5 and No.6. Depends on later usage, we need to see how people use it and gather feedback

These 1-9 points/details we can still discuss, im open to. Lets figure out first with roll-out plan - 1.a-b-c or 2.a-b

@mutantcornholio
Copy link
Author

  1. gradually with minimum-to-0 breaking changes to an arguments side
    a) /cmd help + /cmd bench $ xcm kusama-dev pallet_referenda_fellowship, so for people we just get rid of -c -v stuff and provide a documentation, while it will be still possible to run it even with old way.
    b) we build UI and for most of the cases you don't need to use ChatBot experience from GH comments
    c) we migrate the arguments API and not ChatBot will look like this /cmd bench cumulus-starters -t xcm, while on UI the change won't be noticeable
  2. introduce breaking change
    a) /cmd help + /cmd bench cumulus-starters -t xcm
    b) build UI as an addition to ChatBot experience

With 1st for me what's valuable is that it still gives nicer experience but doesn't require you to learn whole new way of how to run it now. While later with UI - some people will have alternative options. With 2nd, we reduce step 1.c) and implement correct schema straight away. The benefit for us - less work later, however I feel that we will have to take care about learning curve, and handle complaints "command bot doesn't respond to me"

To summarize - i'd go with named arguments, how you suggest, but through 1st, gradual steps, right after the UI as an alternative is provided

Wait, so you're suggesting to do the refactor twice, in order to postpone breaking changes, based on the premise that people who're using GitHub comments now will switch to UI, so less people would be affected by the breaking changes, while switching to UI is far more of a breaking change?

You know well my standpoint on this by now: do a switch once. Teach and support users during the transition. I'd wager that it would be much more straightforward process and less work overall.

@mutantcornholio
Copy link
Author

  • if I understand correctly - passing the REPOSITORY makes sense, so you don't infer it from the folder name. However I didn't get full why the shell script need a preset ? May be would be good to discuss via voice

I'm not sure that it would be a right call, but I can imagine arguments created specifically to represent a preset.
Probably makes sense to see how the scripts would evolve first.

@mordamax
Copy link
Collaborator

superseded with #2

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