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: add ion template inspect command #11

Merged
merged 10 commits into from
Feb 16, 2023
Merged

feat: add ion template inspect command #11

merged 10 commits into from
Feb 16, 2023

Conversation

diversable
Copy link
Contributor

The template command(s) have been tested and are working; hopefully the style somewhat keeps with the Ion repo.

You may want to check out the 'clone' tests, though - I'm getting intermittent failures with that test, and can't figure out why yet (nothing I did should've had any impact on the bin::clone::test_clone test). Just thought you should know, if you weren't already aware...

Closes issue: #9 (comment)

NB: updates have been made to deal with the non-happy-path; as of now, if the template the user wants to inspect does not exist in the list of templates installed, Ion will present a list of installed templates for the user to choose from..

Copy link
Owner

@Roger-luo Roger-luo left a comment

Choose a reason for hiding this comment

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

Looks good in general, only a small change is needed thanks!

Comment on lines 206 to 216
let template_name = Select::with_theme(&ColorfulTheme::default())
.items(&selection_options)
.default(1)
.interact_opt()
.unwrap();

match template_name {
Some(index) => inspect_template(config, selection_options[index].to_owned())?,
_ => unreachable!(),
}
Ok(())
Copy link
Owner

Choose a reason for hiding this comment

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

should be an if let Some(...) = ...? and the select error should be thrown to with ? to that user can exit without backtrace and exit code


let path = entry.path();
if path.is_dir() {
let source = std::fs::read_to_string(path.join("template.toml"))?;
Copy link
Owner

Choose a reason for hiding this comment

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

we probably should just provide a pretty-printing for Template so we can just use that here. But I think directly print the source is also good, we can improve it later.

Cargo.toml Outdated Show resolved Hide resolved
@diversable
Copy link
Contributor Author

Thanks for the feedback!

I agree that I/we should impl display for other template attributes so that we can print out a more 'detailed inspection report' of the requested template.

For the current implementation, I was trying to think from the user's perspective: mostly what I need/want is to get an overview of what's going to be created for a template. Printing the source was sufficient to meet that requirement, but more details are available in the template struct.

So, I was thinking of adding a flag called either

  1. --detail or
  2. --long or
  3. '--full
    which would pretty-print the specifics of a given template, as you suggest.

Do you concur with this approach? If so, do you have a flag name preference?
Or should we just print out the detailed inspection report instead of the more succinct source code?

@Roger-luo
Copy link
Owner

yeah, that sounds good, I think usually the convention is -v, --verbose for printing detailed information? I think this PR should be good to merge tho, I'm gonna merge just after CI passes.

Copy link
Owner

@Roger-luo Roger-luo left a comment

Choose a reason for hiding this comment

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

Sorry one last change is I think we should have some basic tests on the interactive select

Comment on lines +206 to +209
let template_name = Select::with_theme(&ColorfulTheme::default())
.items(&selection_options)
.default(1)
.interact_opt()?;
Copy link
Owner

Choose a reason for hiding this comment

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

Is this not tested in the integration test? There is a PtySession tool that can be used to test interactive processes using spawn: https://github.com/Roger-luo/Ion/blob/main/tests/bin/utils.rs#L47

an example you look at is the ion new command's test https://github.com/Roger-luo/Ion/blob/main/tests/bin/new.rs#L18

Copy link
Contributor Author

@diversable diversable Feb 16, 2023

Choose a reason for hiding this comment

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

Please forgive me - I'm not an expert in Rust CLI testing, though I am eager to learn.

I'm having difficulty figuring out how to send the [ENTER] or [SPACE] keycodes to the command line with Ion's pty tool, which is required to step through the Templates' test interaction: just calling an empty send("") or sendline("") doesn't work, since it's sending a string type, which doesn't work to send raw keycodes & thus to test Dialoguer's Select functionality (which only accepts [ENTER] or [SPACE] in user interaction).

There doesn't (currently) seem to be a way to send raw keycodes to Ion's Pty...

Or is there something I'm missing? Or do I just need to dig deeper into and add to the Pty implementation so that we can send key codes as well as strings to the Pty session during testing?

Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(p.s. I'll have fewer of these silly questions as I get more familiar with working on Ion's codebase. Thank you for your help as I get up to speed!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind - fixed it.

I guess we can't use the Ctrl+M keycode to enter return keycodes for Dialoguer Select fields, but Ctrl+J works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with latest commit

Copy link
Owner

Choose a reason for hiding this comment

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

yeah it's the send control method, thanks!

@Roger-luo Roger-luo merged commit 592c434 into Roger-luo:main Feb 16, 2023
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