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

Support tab completion for variables. #304

Merged
merged 1 commit into from
Jun 21, 2017
Merged

Conversation

georgebrock
Copy link
Collaborator

This commit introduces a Gitsh::TabCompletion::VariableCompleter object. This is completely separate from the NFA-based completion we use for most things, because:

  • we immediately know we need to use variable completion based on lexical analysis of the input without having to walk through the NFA's states, and

  • the context in which the variable appears doesn't inform the completions we present the user with.

The Gitsh::TabCompletion::Facade object gains the responsibility of deciding which type of completion to invoke, based on information from the Gitsh::TabCompletion::Context object.

@georgebrock georgebrock force-pushed the variable-completion branch 2 times, most recently from 465238a to a5d6beb Compare June 17, 2017 17:09
@georgebrock
Copy link
Collaborator Author

Travis CI is using a very old version of Git (1.8.5.6) which doesn't support the git config --list --name-only command used by this PR.

However, the --name-only argument was introduced in version 2.6, and versions back to 2.4 are still officially supported.

The --name-only release notes indicate that it was introduced because parsing git config --list output is hard when there are multi-line config values. That makes me think we should probably try --name-only and fall back to attempting to parse --list.

Copy link
Contributor

@sharplet sharplet left a comment

Choose a reason for hiding this comment

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

I read through the implementation and scanned the tests — here's a couple of minor comments. Looking good 👍


def old_git_available_config_variables
git_output('config --list').lines.map do |line|
line.split('=').first.to_sym
Copy link
Contributor

Choose a reason for hiding this comment

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

This could use line.upto('=').to_sym to avoid scanning the entire line.

Copy link
Collaborator Author

@georgebrock georgebrock Jun 19, 2017

Choose a reason for hiding this comment

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

Sadly, that's not what String#upto does (e.g. 'a'.upto('d') produces an enumerator over ['a', 'b', 'c', 'd']). Your version seems more useful though.

I could use split('=', 2).first, that should avoid scanning the whole thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm...given that #split still returns an array in that case, it's unclear whether that's improving things much. The best alternative I could come up with was

line.each_char.take_while { |c| c != "-" }.join

But given that config lines aren't usually very long (I assume), it's probably not a big deal either way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I'll stick with split.first.

out, _, status = Open3.capture3(command)

if status.success?
out.chomp.lines.map { |line| line.chomp.to_sym }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the first #chomp redundant given that each line is individually chomped?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Excellent point.

Copy link
Contributor

@mike-burns mike-burns left a comment

Choose a reason for hiding this comment

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

This seems right. No complaints. Well, one complaint. No real complaints.

magic_variables.available_variables +
variables.keys +
repo.available_config_variables
).sort.uniq
Copy link
Contributor

Choose a reason for hiding this comment

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

For the most micro of micro optimizations, you could uniq.sort -- this will cause sort to have a smaller list to work through.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We'll save so much time over the next 50 years!

This commit introduces a `Gitsh::TabCompletion::VariableCompleter` object.
This is completely separate from the NFA-based completion we use for most
things, because:

- we immediately know we need to use variable completion based on lexical
  analysis of the input without having to walk through the NFA's states,
  and

- the context in which the variable appears doesn't inform the completions
  we present the user with.

The `Gitsh::TabCompletion::Facade` object gains the responsibility of
deciding which type of completion to invoke, based on information from the
`Gitsh::TabCompletion::Context` object.

To load the config variables, we try to use the `git status --list
--name-only` command. However, this command was only introduced in Git 2.6,
and versions back to 2.4 are still officially supported by the Git team. If
running the command with `--name-only` fails we fall back to parsing the
output of `git status --list`.

We don't use the backward compatible version everywhere, because the parsing
won't be perfect for multi-line config values (`--name-only` was introduced
to address this problem).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants