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: support shell completions in tedge cli #3332

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

jarhodes314
Copy link
Contributor

@jarhodes314 jarhodes314 commented Jan 15, 2025

Proposed changes

This adds shell completions (bash, zsh, fish) to the tedge CLI. It should be fairly self explanatory, and the documentation added in the PR should explain how you can enable the feature for testing locally.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

Copy link

codecov bot commented Jan 15, 2025

Copy link
Contributor

github-actions bot commented Jan 16, 2025

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
552 0 2 552 100 1h28m42.141327999s

Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Testing this PR with zsh I noticed some weirdness, however overall completion is working nicely.

Completion on tedge config set is fantastic!

The weirdness seems to be due to the overly complicated argument logic of thin-edge.

  • e.g. --config-dir, --debug, --log-level are never proposed for completion

Comment on lines +105 to +112
fn qos_completions() -> Vec<CompletionCandidate> {
vec![
CompletionCandidate::new("0").help(Some("At most once".into())),
CompletionCandidate::new("1").help(Some("At least once".into())),
CompletionCandidate::new("2").help(Some("Exactly once".into())),
]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Really nice

$ tedge mqtt pub --qos <TAB>
0  -- At most once
1  -- At least once
2  -- Exactly once

Comment on lines +41 to 42
#[clap(long = "output-path", global = true, value_hint = ValueHint::FilePath)]
output_path: Option<Utf8PathBuf>,
Copy link
Contributor

Choose a reason for hiding this comment

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

We might have to add this hint for other path (e.g. --config-dir ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I also need to make a modification to support the global arguments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the hint and fixed the issue where the global arguments weren't completed

Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance to get the --log-level value also completed? e.g. tedge --log-level <TAB><TAB>

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, works as expected

@jarhodes314
Copy link
Contributor Author

The weirdness seems to be due to the overly complicated argument logic of thin-edge.

  • e.g. --config-dir, --debug, --log-level are never proposed for completion

Now fixed

Copy link
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

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

Approving (from a UX point of view). Still wait for a Rust review.

Really nice addition. We can work out any packaging aspects in a follow up PR

Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Approved. Quickly and nicely done.

Comment on lines +171 to +173
/// This will infer the profile names from the various cloud configurations.
/// It would be significantly more complicated to try and do per-cloud
/// completions, and would likely provide no real value to anyone.
Copy link
Contributor

Choose a reason for hiding this comment

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

That seems reasonable.

@jarhodes314 jarhodes314 added this pull request to the merge queue Jan 16, 2025
Merged via the queue into thin-edge:main with commit 03b0756 Jan 16, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme:cli Theme: cli related topics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants