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: avoid auto-installing when already installed #74

Merged
merged 47 commits into from
May 30, 2023

Conversation

alestiago
Copy link
Contributor

@alestiago alestiago commented May 23, 2023

Status

READY

Description

Resolves #56

Should be merged after #73

Changes:

  • Adds installs into CompletionConfiguration
  • Check if already installed before auto-installing
  • Renamed typedef Uinstalls to ShellCommandsMap since it is now used

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

@alestiago alestiago changed the base branch from main to feat/avoid-autoinstalling-uninstalls May 23, 2023 13:25
Comment on lines 85 to 88
/// Stores those commands that have completion installed.
///
/// Installed commands are specific to a given [SystemShell].
final Uninstalls installs;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get we have to keep a map of all commands. If we have this lib installed on a cli, other installations should not concern this routine. In other words, this algorithm will never use more the one element in these collections, especially "installs"

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this "installs" collection necessary?

Copy link
Contributor Author

@alestiago alestiago May 23, 2023

Choose a reason for hiding this comment

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

I'm going to illustrate the necessity of the installs collection with an example and the mathematical interpreation. After reading them, do you have another approach in mind?

Note that as planned off-github, installs and uninstalls are stored on a single "global" configuration file (~/.dart-cli-completion/config.json).


Example

  1. User uses very_good command in ZSH and it gets auto-installed, the installs collection will go from being empty to:
{
  SystemShell.zsh: {"very_good"}
}
  1. User uses dart_frog command in ZSH and it gets auto-installed, the installs collection will now be:
{
  SystemShell.zsh: {"very_good", "dart_frog"}
}
  1. User uses very_good command in in Bash and it gets auto-installed, the installs collection will now be:
{
  SystemShell.zsh: {"very_good", "dart_frog"},
  SystemShell.bash: {"very_good"},
}
  1. User uses very_good command in Zsh and it does not get auto-installed since we know it it already installed.

Mathematical interpretation

The $installs$ set (let it be denoted with $I$) collection is mutually exclusive to the $uninstalls$ set (let it be denoted with $U$); as shown in the Venn Diagram below. It is instructive to know that $I \cup U \neq \text{all commands}$, hence a single collection is not enough. It follows that if and only if auto-install is enabled on all commands: $I \cup U = \text{all commands used so far after cli completion was installed}$.

venn-installs drawio

lib/src/command_runner/completion_command_runner.dart Outdated Show resolved Hide resolved
@alestiago alestiago marked this pull request as draft May 23, 2023 18:00
Base automatically changed from feat/avoid-autoinstalling-uninstalls to main May 23, 2023 19:50
@alestiago alestiago marked this pull request as ready for review May 24, 2023 14:17
@alestiago alestiago merged commit 333371e into main May 30, 2023
@alestiago alestiago deleted the feat/efficient-auto-installs branch May 30, 2023 16:46
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.

feat: Optimize auto install behavior
3 participants