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: Candid assist #3546

Merged
merged 17 commits into from
Feb 16, 2024
Merged

feat: Candid assist #3546

merged 17 commits into from
Feb 16, 2024

Conversation

chenyan-dfinity
Copy link
Contributor

@chenyan-dfinity chenyan-dfinity commented Jan 29, 2024

Description

Ask for user input when candid argument is not provided. A nice side effect is that dfx deploy now works with multiple canisters with init args (we cannot use --argument flag when multiple canisters require init args)

How Has This Been Tested?

e2e test

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@chenyan-dfinity chenyan-dfinity requested a review from a team as a code owner January 29, 2024 18:47
@adamspofford-dfinity
Copy link
Contributor

Not easy to write e2e test, because it's only activated when used in terminal.

DFX has a number of interactive tests in Expect format, in e2e/assets/expect_scripts. See here for an example, may require updating this line.

CHANGELOG.md Show resolved Hide resolved
src/dfx/src/util/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@ericswanson-dfinity ericswanson-dfinity left a comment

Choose a reason for hiding this comment

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

This happens when I run dfx new hello && cd hello && dfx deploy:

Installing code for canister hello_backend, with canister ID by6od-j4aaa-aaaaa-qaadq-cai
✔ Do you want to enter an optional value? · yes
  Enter a value for AssetCanisterArgs : variant { Upgrade : UpgradeArgs; Init : InitArgs }
  ✔ Select a variant · Init
    Enter a value for InitArgs : record {}
Sending the following argument:
(opt variant { Init = record {} })

Do you want to send this message? [y/N]

Expected behavior: deploying the default project doesn't block asking for any parameters.

(Also: it's prompting for the frontend canister, not the backend canister, but there's no indication of that at the "Do you want to enter an optional value?" prompt.)

@chenyan-dfinity
Copy link
Contributor Author

Should be good now. I move the install prompts at the top. It also helps with other warning messages. Restored the behavior of not asking for arguments when all types are optional.

dfx deploy
assert_command "${BATS_TEST_DIRNAME}/../assets/expect_scripts/candid_assist.exp"
}

Choose a reason for hiding this comment

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

Can we have one of these for deploy and install as well?

Here is the output I see when deploying to a canister declared as actor class c(p: { x: Nat; y: Int;}) :

Installing canisters...
Installing code for canister dd_backend, with canister ID bkyz2-fmaaa-aaaaa-qaaaq-cai
Enter field x : nat
  ✔ Enter a nat · 6
Enter field y : int
  ✔ Enter an int · 8
Sending the following argument:
(record { x = 6 : nat; y = 8 : int })

Do you want to send this message? [y/N]

Suggested wording:

Installing canisters...
Installing code for canister dd_backend, with canister ID bkyz2-fmaaa-aaaaa-qaaaq-cai

Canister dd_backend requires an initialization argument.
Enter field x : nat
  ✔ Enter a nat · 6
Enter field y : int
  ✔ Enter an int · 8

Initialization argument for dd_backend will be:
(record { x = 6 : nat; y = 8 : int })

Do you want to initialize canister dd_backend with this argument? [y/N]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Choose a reason for hiding this comment

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

Here's what I see now, when deploying a canister with declaration actor class c(x: Principal) {

Installing code for canister dd_backend, with canister ID bkyz2-fmaaa-aaaaa-qaaaq-cai
This canister requires the following initialization argument.
Auto-completions: alice, anonymous, bob, dd_backend, dd_frontend, default, mainnet
? Enter a principal › 

Note "This canister requires the following initialization argument." but no following description of the argument. The term "requires the following initialization argument" would have to refer to a "following" description of that argument. It can't be used to refer only to the upcoming prompt for that argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed back to "This canister requires an initialization argument."

if !principals.is_empty() {
let mut map = BTreeMap::new();
map.insert("principal".to_string(), principals);
ctx.set_completion(map);

Choose a reason for hiding this comment

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

This set_completion call overwrites the completions, which makes it so anonymous doesn't show up in the list of autocompletion principals. Is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's intended. We probably need to add anonymous manually in the identity export.

@chenyan-dfinity chenyan-dfinity enabled auto-merge (squash) February 16, 2024 18:51
@chenyan-dfinity chenyan-dfinity merged commit df4b628 into master Feb 16, 2024
175 checks passed
@chenyan-dfinity chenyan-dfinity deleted the candid-assist branch February 16, 2024 19:44
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.

3 participants