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

Make scarb new and init command interactive #1472

Merged
merged 7 commits into from
Jul 29, 2024

Conversation

ksew1
Copy link
Member

@ksew1 ksew1 commented Jul 23, 2024

Closes #1207
Closes #1982

Make scarb new and init command interactive

scarb/src/bin/scarb/args.rs Outdated Show resolved Hide resolved
scarb/src/bin/scarb/commands/new.rs Outdated Show resolved Hide resolved
scarb/tests/new_and_init.rs Outdated Show resolved Hide resolved
scarb/src/bin/scarb/commands/init.rs Outdated Show resolved Hide resolved
scarb/src/bin/scarb/interactive.rs Outdated Show resolved Hide resolved
@maciektr
Copy link
Contributor

Would you be able to add any tests for that?

@ksew1 ksew1 force-pushed the scarb-new-interactive branch 2 times, most recently from 3a87409 to a48e85a Compare July 25, 2024 10:33
@ksew1 ksew1 force-pushed the scarb-new-interactive branch from a48e85a to ea12162 Compare July 25, 2024 11:12
@ksew1
Copy link
Member Author

ksew1 commented Jul 25, 2024

Would you be able to add any tests for that?

Regarding the tests, the task is harder than it seemed. We can't run this as std::process::Command as it's not TTY. I tried PtyProcess, which works on my computer but not on CI. Additionally, this is platform-dependent, so it won't build on Windows. I don't know if it is really worth it.

We can write unit tests by adjusting the function signature as follows:

pub fn ask_for_test_runner<F>(prompt: F) -> Result<TestRunner>
where
    F: FnOnce(Vec<&str>) -> InquireResult<&str>

and then mocking the prompt.

However, if you think e2e tests are really important, I can try to research this topic further and maybe find a solution to run them.

@maciektr
Copy link
Contributor

That's why I asked 😃

If it's too hard, I can live without tests (is this a problem with getting output only, or are the files not created on CI? 🤔 ). Unit approach is not worth it imho, as it mocks exactly what we would want to test in this scenario.

@ksew1 ksew1 marked this pull request as draft July 25, 2024 13:54
@ksew1
Copy link
Member Author

ksew1 commented Jul 25, 2024

That's why I asked 😃

If it's too hard, I can live without tests (is this a problem with getting output only, or are the files not created on CI? 🤔 ). Unit approach is not worth it imho, as it mocks exactly what we would want to test in this scenario.

The issue is getting input; it seems it is incorrectly passed. I haven't debugged it, though.

@ksew1 ksew1 force-pushed the scarb-new-interactive branch 2 times, most recently from c44da4b to 768f592 Compare July 26, 2024 08:42
@ksew1 ksew1 marked this pull request as ready for review July 26, 2024 08:42
Cargo.toml Outdated Show resolved Hide resolved
scarb-metadata/tests/command.rs Outdated Show resolved Hide resolved
scarb/src/bin/scarb/args.rs Outdated Show resolved Hide resolved
scarb/src/bin/scarb/commands/new.rs Show resolved Hide resolved
scarb/src/bin/scarb/commands/init.rs Outdated Show resolved Hide resolved
@ksew1 ksew1 requested a review from mkaput July 26, 2024 11:14
scarb/src/bin/scarb/interactive.rs Outdated Show resolved Hide resolved
scarb/src/bin/scarb/interactive.rs Show resolved Hide resolved
@ksew1 ksew1 requested a review from maciektr July 26, 2024 13:15
Copy link
Contributor

@maciektr maciektr left a comment

Choose a reason for hiding this comment

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

Please make sure to use the new env var in snforge init after this is merged :D

scarb/src/internal/serdex.rs Outdated Show resolved Hide resolved
website/docs/writing-extensions/subcommands.md Outdated Show resolved Hide resolved
@ksew1 ksew1 force-pushed the scarb-new-interactive branch from 64283a0 to c3e51b3 Compare July 26, 2024 13:55
@ksew1 ksew1 requested a review from maciektr July 26, 2024 14:45
@maciektr maciektr added this pull request to the merge queue Jul 29, 2024
Merged via the queue into software-mansion:main with commit 51f7015 Jul 29, 2024
22 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.

Scarb foundry project template Make scarb new command interactive
3 participants