Tab completion NFA DSL #310

georgebrock · 2017-06-30T14:42:44Z

#296 introduced a non-deterministic finite automaton as the basis of the tab completion system. This PR is the next step in my grand plan for tab completion: it introduces a DSL for loading the NFA's state graph from a configuration file.

The DSL format is described in the gitsh_completions.5 manual page, which is added in this PR.

The configuration file included in this PR (etc/completions) results in a graph very similar to the one that was hard coded prior to the introduction of the DSL. Subsequent PRs will add features to the DSL so it can be used for more complex graphs that produce better tab completion behaviour.

Note that I'm not using DSL in the sense it's usually used with Ruby: this language has a lexer and a parser, it's not just a pretty looking Ruby API.

houndci-bot · 2017-06-30T14:43:04Z

lib/gitsh/tab_completion/dsl/lexer.rb

+        rule(/\s+/) { }
+
+        rule(/#/) { push_state :comment }
+        rule(/[^\n]+/, :comment) { }


Space inside empty braces detected.

houndci-bot · 2017-06-30T14:43:04Z

lib/gitsh/tab_completion/dsl/lexer.rb

+        rule(/\(/) { :LEFT_PAREN }
+        rule(/\)/) { :RIGHT_PAREN }
+        rule(/\s*\n\s*\n/) { :BLANK }
+        rule(/\s+/) { }


Space inside empty braces detected.

houndci-bot · 2017-06-30T14:43:04Z

lib/gitsh/tab_completion/dsl/concatenation_factory.rb

+
+        def build(start_state, options = {})
+          with_optional_end_state(options) do
+            parts.inject(start_state) do |state, part|


Prefer reduce over inject.

But that's not what I'm here to tell you about.
I'm here to talk about the draft.
They got a buildin' down in New York City called Whitehall Street, where you
Walk in, you get injected, inspected, detected, infected, neglected and
Selected!

Hound config updated.

houndci-bot · 2017-06-30T14:44:47Z

spec/units/tab_completion/dsl/rule_set_factory_spec.rb

+    it 'delegates to the various rule factories' do
+      start_state = double(:start_state)
+      rule_factory_1 = stub_rule_factory
+      rule_factory_2 = stub_rule_factory


Use normalcase for variable numbers.

houndci-bot · 2017-06-30T14:44:47Z

spec/units/tab_completion/dsl/rule_set_factory_spec.rb

+  describe '#build' do
+    it 'delegates to the various rule factories' do
+      start_state = double(:start_state)
+      rule_factory_1 = stub_rule_factory


Use normalcase for variable numbers.

Hound config updated.

houndci-bot · 2017-06-30T14:44:47Z

spec/units/tab_completion/dsl/parser_spec.rb

+      result = described_class.parse(tokens(
+        [:WORD, 'push'], [:BLANK],
+        [:WORD, 'pull'], [:BLANK], [:BLANK],
+        [:WORD, 'fetch'], [:EOS],


Avoid comma after the last parameter of a method call, unless each item is on its own line.

houndci-bot · 2017-06-30T14:44:47Z

spec/units/tab_completion/dsl/parser_spec.rb

+
+    it 'parses multiple rules in the same input' do
+      result = described_class.parse(tokens(
+        [:WORD, 'push'], [:BLANK],


Indent the first parameter one step more than tokens(.

houndci-bot · 2017-06-30T14:44:47Z

spec/units/tab_completion/dsl/parser_spec.rb

+    it 'parses rules combining the pipe operator and a post-fix operator' do
+      result = parse_single_rule(tokens(
+        [:LEFT_PAREN], [:WORD, 'add'], [:OR], [:WORD, 'commit'], [:RIGHT_PAREN],
+        [:PLUS], [:EOS],


Avoid comma after the last parameter of a method call, unless each item is on its own line.

houndci-bot · 2017-06-30T14:44:48Z

spec/units/tab_completion/dsl/lexer_spec.rb

+
+    it 'recognises parentheses' do
+      expect('(add|commit)').to produce_tokens [
+        'LEFT_PAREN', 'WORD(add)', 'OR', 'WORD(commit)', 'RIGHT_PAREN', 'EOS',


Avoid comma after the last item of an array, unless each item is on its own line.

houndci-bot · 2017-06-30T14:44:48Z

spec/units/tab_completion/dsl/lexer_spec.rb

+
+    it 'recognises the pipe operator' do
+      expect('stash pop|drop').to produce_tokens [
+        'WORD(stash)', 'WORD(pop)', 'OR', 'WORD(drop)', 'EOS',


Avoid comma after the last item of an array, unless each item is on its own line.

houndci-bot · 2017-06-30T14:44:48Z

spec/units/tab_completion/dsl/concatenation_factory_spec.rb

+        end_state_1 = double(:end_state_1)
+        part_1 = double(:factory, build: end_state_1)
+        end_state_2 = double(:end_state_2, add_free_transition: nil)
+        part_2 = double(:factory, build: end_state_2)


Use normalcase for variable numbers.

houndci-bot · 2017-06-30T14:44:48Z

spec/units/tab_completion/dsl/concatenation_factory_spec.rb

+        end_state = double(:end_state)
+        end_state_1 = double(:end_state_1)
+        part_1 = double(:factory, build: end_state_1)
+        end_state_2 = double(:end_state_2, add_free_transition: nil)


Use normalcase for variable numbers.

houndci-bot · 2017-06-30T14:44:48Z

spec/units/tab_completion/dsl/concatenation_factory_spec.rb

+        start_state = double(:start_state)
+        end_state = double(:end_state)
+        end_state_1 = double(:end_state_1)
+        part_1 = double(:factory, build: end_state_1)


Use normalcase for variable numbers.

mike-burns

I don't love GITSH_CONFIG_DIRECTORY, but I also don't want to make you pass a string through a series of method calls.

Overall: LGTM!

mike-burns · 2017-06-30T15:18:23Z

lib/gitsh/tab_completion/dsl.rb

+        factory = Parser.parse(tokens, gitsh_env: env)
+        factory.build(start_state)
+      rescue Errno::ENOENT
+      end


The silence here makes me suspect that we want a conditional elsewhere instead. I feel like DSL.load should complain about a missing file, but that DSL.load should only be called if we want such a complaint ... if that makes sense.

I could move the rescue up a level to the Gitsh::TabCompletion::AutomatonFactory object?

I don't think I want a missing configuration file to cause an error that the user sees, but maybe it should—without the file there will be no tab completion.

mike-burns · 2017-06-30T15:22:41Z

lib/gitsh/tab_completion/dsl/concatenation_factory.rb

+
+        def build(start_state, options = {})
+          with_optional_end_state(options) do
+            parts.inject(start_state) do |state, part|


But that's not what I'm here to tell you about.
I'm here to talk about the draft.
They got a buildin' down in New York City called Whitehall Street, where you
Walk in, you get injected, inspected, detected, infected, neglected and
Selected!

mike-burns · 2017-06-30T15:35:31Z

man/man1/gitsh.1.in

@@ -406,6 +406,13 @@ The
 .Pa .gitshrc
 file will not be loaded for non-interactive sessions, e.g. when gitsh is
 invoked with the path to a script file.
+.It Pa @prefix@/etc/gitsh/completions


Maybe @pkgsysconfdir@ instead of @prefix@ is more accurate.

mike-burns · 2017-06-30T15:37:56Z

man/man5/gitsh_completions.5.in

+file consists of rules defining the arguments that are expected
+by various commands.
+.Xr gitsh 1
+uses the rules to match the input before the word being completed,


Probably no comma.

mike-burns · 2017-06-30T15:38:13Z

man/man5/gitsh_completions.5.in

+.Pp
+Each rule is separated from other rules by a blank line.
+.Pp
+The rule specifies the name of the command,


mike-burns · 2017-06-30T15:46:50Z

spec/support/tab_completion.rb

+  def stub_text_matcher(text)
+    klass = Gitsh::TabCompletion::Matchers::TextMatcher
+    matcher = instance_double(klass)
+    allow(klass).to receive(:new).with(text).and_return(matcher)


#with makes this into a mock instead of a stub, right?

I never get those distinctions right, but you're probably right.

mike-burns · 2017-06-30T15:51:14Z

spec/units/tab_completion/dsl_spec.rb

+  end
+
+  def write_temp_completions_file(completions)
+    temp_file('gitsh_completions', completions).path


How's this work -- do these files get cleaned up?

Yes. It uses the Ruby standard library's Tempfile class behind the scenes, so it should be cleaned up when the object is garbage collected.

georgebrock · 2017-06-30T17:37:32Z

I don't love GITSH_CONFIG_DIRECTORY, but I also don't want to make you pass a string through a series of method calls.

It might not be too bad: the file it's current defined in would pass it to Gitsh::CLI, to Gitsh::InputStrategies::Interactive, to Gitsh::TabCompletion::Facade, to Gitsh::TabCompletion::AutomatonFactory where it's used.

georgebrock · 2017-06-30T17:52:20Z

Thinking about this configuration thing some more: we already pass the Gitsh::Environment instance around all of these places, and it's already able to hold variables. If we put this path in a variable, that would allow people to override it in their .gitshrc file.

houndci-bot · 2017-06-30T18:20:47Z

spec/units/tab_completion/dsl_spec.rb

+    it 'builds a graph of automaton states by reading the given file' do
+      path = write_temp_completions_file([
+        'stash (apply|drop|pop|show)',
+      ].join("\n"))


Put a comma after the last parameter of a multiline method call.

Err … this isn't quite what I intended. Rubocop seems to struggle with nested multiline things.

houndci-bot · 2017-06-30T18:20:47Z

spec/units/tab_completion/dsl/parser_spec.rb

+        [:WORD, 'push'], [:BLANK],
+        [:WORD, 'pull'], [:BLANK], [:BLANK],
+        [:WORD, 'fetch'], [:EOS],
+      ), gitsh_env: double(:env))


Put a comma after the last parameter of a multiline method call.

houndci-bot · 2017-06-30T18:20:47Z

spec/units/tab_completion/dsl/parser_spec.rb

+      result = parse_single_rule(tokens(
+        [:LEFT_PAREN], [:WORD, 'add'], [:OR], [:WORD, 'commit'], [:RIGHT_PAREN],
+        [:PLUS], [:EOS],
+      ))


Put a comma after the last parameter of a multiline method call.

houndci-bot · 2017-06-30T18:20:47Z

spec/units/tab_completion/dsl/parser_spec.rb

+      result = parse_single_rule(tokens(
+        [:LEFT_PAREN], [:WORD, 'stash'], [:WORD, 'pop'], [:OR],
+        [:WORD, 'add'], [:RIGHT_PAREN], [:EOS],
+      ))


Put a comma after the last parameter of a multiline method call.

houndci-bot · 2017-06-30T18:20:47Z

spec/units/tab_completion/dsl/parser_spec.rb

+      result = parse_single_rule(tokens(
+        [:LEFT_PAREN], [:WORD, 'commit'], [:OR],
+        [:WORD, 'add'], [:OR], [:VAR, 'path'], [:RIGHT_PAREN], [:EOS],
+      ))


Put a comma after the last parameter of a multiline method call.

houndci-bot · 2017-06-30T18:20:48Z

spec/units/tab_completion/dsl/parser_spec.rb

+    it 'parses rules with multiple words and variables' do
+      result = parse_single_rule(tokens(
+        [:WORD, 'stash'], [:WORD, 'pop'], [:VAR, 'revision'], [:EOS],
+      ))


Put a comma after the last parameter of a multiline method call.

houndci-bot · 2017-06-30T18:20:48Z

lib/gitsh/tab_completion/matchers/path_matcher.rb

@@ -4,6 +4,9 @@ module Gitsh
  module TabCompletion
    module Matchers
      class PathMatcher < BaseMatcher
+        def initialize(_env)


Put empty method definitions on a single line.

houndci-bot · 2017-07-03T15:01:05Z

lib/gitsh/environment.rb

@@ -6,8 +6,9 @@
 module Gitsh
  class Environment
    DEFAULT_GIT_COMMAND = '/usr/bin/env git'.freeze
+    DEFAULT_CONFIG_DIRECTORY = '/usr/local/etc/gitsh'


Freeze mutable objects assigned to constants.

georgebrock · 2017-07-03T15:03:10Z

@mike-burns I think 3a76b3f addresses your concerns about the global constant.

mike-burns · 2017-07-03T15:04:25Z

Sure does!

LGTM.

This commit makes the Rubocop config used by Hound more consistent with the existing project style.

This commit introduces a domain specific language (DSL) to define the state graph for the tab completion automaton. It also replaces the hard-coded state graph with an equivalent graph loaded from a configuration file. The details of the DSL are given in the gitsh_completions(5) manual page which is added in this commit. The DSL will need to be expanded somewhat to do everything we want the tab completion system to be able to do, but this is a good step in that direction.

houndci-bot reviewed Jun 30, 2017

View reviewed changes

mike-burns reviewed Jun 30, 2017

View reviewed changes

houndci-bot reviewed Jun 30, 2017

View reviewed changes

georgebrock force-pushed the nfa-dsl-omg-redux branch from a867127 to 39dc722 Compare July 3, 2017 14:37

houndci-bot reviewed Jul 3, 2017

View reviewed changes

georgebrock added 2 commits July 3, 2017 11:13

Update hound config.

8cf52c0

This commit makes the Rubocop config used by Hound more consistent with the existing project style.

georgebrock force-pushed the nfa-dsl-omg-redux branch from c3a8d2e to 69ae4f2 Compare July 3, 2017 15:17

georgebrock merged commit 69ae4f2 into master Jul 3, 2017

This was referenced Aug 11, 2017

Add tab completion for options #80

Closed

Auto complete stash commands #220

Closed

Support the same tab completion options as zsh #25

Closed

Tab completion NFA DSL #310

Tab completion NFA DSL #310

Conversation

georgebrock commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-burns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgebrock commented Jun 30, 2017

georgebrock commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgebrock commented Jul 3, 2017

mike-burns commented Jul 3, 2017