Introduce an object registry #371

georgebrock · 2019-08-03T17:59:13Z

What?

This PR is a large refactor which replaces dependency injection with the registry pattern for shared instances, or direct references to class names that were previously passed as constructor arguments.

Previously we had a lot of classes that looked like this:

class Foo
  def initialize(env, bar_factory = Gitsh::Bar)
    @bar = bar_factory.new(env)
    @env = env
  end

  private

  attr_reader :bar, :env
end

Now, those same classes look like this:

class Foo
  extend Registry::Client
  use_registry_for :env

  def initialize
    @bar = Gitsh::Bar.new
  end

  private

  attr_reader :bar
end

The objects moved to the registry are the shared instances of:

Gitsh::Environment
Gitsh::GitRepository
Gitsh::LineEditorHistoryFilter (decorating Gitsh::LineEditor)

Why?

The nature of gitsh means that some amount of long-lasting, global state is inevitable. Unlike a Web application, which needs to handle multiple concurrent requests from different users, a gitsh process only runs a single command at any given time, and does so for a single user in a single session.

We get a lot of benefits from this approach, which include:

Fewer redundant arguments

There were several classes which held a reference to the shared Gitsh::Environment instance only because they needed to pass it along to some other object that needed to use it.
Easier to break up Gitsh::Environment

The Gitsh::Environment has become a bit of a junk drawer. The main reason is that it was the one object that was available everywhere, so access to any shared state was easiest if it went through this object.

Introducing the registry means we are able to drop all of the Gitsh::Environment#repo_* methods (which delegated directly to a Gitsh::GitRepository instance) and instead share both the Gitsh::Environment and Gitsh::GitRepository via the registry.

The remaining responsibilities of Gitsh::Environment (I/O, variable access, and access to some global config) can be broken out in future PRs.
Easier to share other instances

The final straw in doing this refactor was starting work on globbing patterns (Wildcards don't seem to work correctly #145), and realising we will need to access the tab completion automaton while evaluating variable values. Adding yet another thing to the environment was not appealing, nor was passing another instance around through the whole system, or building multiple instances of a pretty large tab completion graph.

After this refactor, we'll be able to use the registry to share the tab completion automaton instance, and easily access it directly from where it's needed.

Code review

This diff is very large (~1,000 changed lines), but many of the changes are very similar and reviewers should probably focus on:

The Gitsh::Registry implementation in lib/gitsh/registry.rb
The new test helpers in spec/support/registry.rb

Instead of passing around `env` absolutely everywhere, let's make it available globally in a single, well-defined place. We can't use a singleton for this, because we do sometimes need to create different, modified environments, e.g. subshells.

hound · 2019-08-03T17:59:32Z

spec/units/prompter_spec.rb

+          initialized?: true,
+          has_modified_files?: true,
+          has_untracked_files?: false,
+        ))


Style/TrailingCommaInArguments: Put a comma after the last parameter of a multiline method call.

hound · 2019-08-03T17:59:32Z

spec/units/prompter_spec.rb

-          format: "%c",
-        )
-        prompter = Gitsh::Prompter.new(env: env)
+        set_registered_env_value('gitsh.prompt', "%c")


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2019-08-03T17:59:32Z

spec/units/prompter_spec.rb

+          initialized?: true,
+          has_modified_files?: true,
+          has_untracked_files?: false,
+        ))


Style/TrailingCommaInArguments: Put a comma after the last parameter of a multiline method call.

hound · 2019-08-03T17:59:32Z

spec/units/prompter_spec.rb

-        prompter = Gitsh::Prompter.new(env: env)
+        set_registered_env_value('gitsh.prompt', "%#")
+        register_repo(status: double(
+          "Status",


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2019-08-03T17:59:32Z

spec/units/prompter_spec.rb

-          format: "%#",
-        )
-        prompter = Gitsh::Prompter.new(env: env)
+        set_registered_env_value('gitsh.prompt', "%#")


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

hound · 2019-08-03T17:59:33Z

spec/units/tab_completion/variable_completer_spec.rb

-          build_env(variables: ['user.name', 'user.email', 'greeting']),
+        register_line_editor
+        register_env(
+          available_variables: [:'user.name', :'user.email', :'greeting'],


Style/SymbolArray: Use %i or %I for an array of symbols.
Style/SymbolLiteral: Do not use strings for word-like symbol literals.

hound · 2019-08-03T17:59:34Z

spec/units/input_strategies/interactive_spec.rb


      input_strategy.handle_parse_error('my message')

-      expect(env).to have_received(:puts_error).with('gitsh: my message')
+      expect(registered_env).to have_received(:puts_error).with('gitsh: my message')


Metrics/LineLength: Line is too long. [84/80]

hound · 2019-08-03T17:59:34Z

spec/units/input_strategies/interactive_spec.rb

-      input_strategy = build_input_strategy
-      allow(line_editor).to receive(:readline).and_return('user input')
+      input_strategy = described_class.new
+      allow(registered_line_editor).to receive(:readline).and_return('user input')


Metrics/LineLength: Line is too long. [82/80]

hound · 2019-08-03T17:59:34Z

spec/units/input_strategies/interactive_spec.rb

      line_editor_results = StubbedMethodResult.new.
        raises(Interrupt).
        returns('user input after interrupt')
-      allow(line_editor).to receive(:readline) { line_editor_results.next_result }
+      allow(registered_line_editor).to receive(:readline) { line_editor_results.next_result }


Metrics/LineLength: Line is too long. [93/80]

hound · 2019-08-03T17:59:34Z

spec/units/input_strategies/interactive_spec.rb

-      input_strategy = build_input_strategy
-      allow(line_editor).to receive(:readline).and_return('user input')
+      input_strategy = described_class.new
+      allow(registered_line_editor).to receive(:readline).and_return('user input')


Metrics/LineLength: Line is too long. [82/80]

georgebrock added 30 commits August 3, 2019 11:32

Introduce the object registry pattern.

9b39bfd

Instead of passing around `env` absolutely everywhere, let's make it available globally in a single, well-defined place. We can't use a singleton for this, because we do sometimes need to create different, modified environments, e.g. subshells.

Use Registry.env in InputStrategies::Interactive

609d7be

Use Registry.env in History

881b0b0

fixup! Use Registry.env in InputStrategies::Interactive

273680b

Use Registry.env in TabCompletion::Facade

11ce21b

Use Registry.env in TabCompletion::AutomatonFactory

d1ffa83

Use Registry.env in TabCompletion::DSL

ec297e7

Use Registry.env in TabCompletion::DSL::Parser

053aaa9

Use Registry.env in TabCompletion::Matchers::*

b19de01

Use Registry.env in TabCompletion::AliasExpander

64fa06f

Use Registry.env in TabCompletion::VariableCompleter

db2932c

Use Registry.env in PromptColor

5188a88

Use Registry.env in Prompter

9bf7794

Use Registry.env in InputStrategies::FileRunner

3cf3c67

Meta-programming for registry client methods.

d4794b8

Use Registry.env in GitRepository

be425c3

Use Registry.env in GitCommandList

f9a36ba

Simplify: remove unused initialize argument.

1601862

Add a GitRepository to the Registry.

757c8ba

Use Registry.repo in Environment

cda7f59

Remove all the Env -> Repo delegates.

49424d8

fixup! Use Registry.env in InputStrategies::Interactive

d59529e

Move line editor to registry.

7aed129

Simplify CLI initializer.

69c1aed

Simplify Interpreter initializer.

281a987

Simplify interactive input strategy initializer.

22b620a

Update bin/tcviz to use registry.

37f499c

Simplify Environment initializer.

f5a1cc1

Use line editor from registry in tab completion.

fe43ceb

Remove unused initialize arg.

9d4f645

georgebrock added 9 commits August 3, 2019 11:32

fixup! Use line editor from registry in tab completion.

7c20c56

Stop injecting constants into things.

d1be651

Use keyword args.

1c34f25

fixup! Use line editor from registry in tab completion.

69cd772

More consistent registry use in specs.

4cb690b

DRY registry population code.

8a77f2b

Clean up registry interface.

a2d60a4

fixup! Simplify interactive input strategy initializer.

910e689

fixup! Stop injecting constants into things.

80cca72

hound bot reviewed Aug 3, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce an object registry #371

Introduce an object registry #371

georgebrock commented Aug 3, 2019

hound bot Aug 3, 2019

hound bot Aug 3, 2019

hound bot Aug 3, 2019

hound bot Aug 3, 2019

hound bot Aug 3, 2019

hound bot Aug 3, 2019

hound bot Aug 3, 2019

hound bot Aug 3, 2019

hound bot Aug 3, 2019

hound bot Aug 3, 2019

Introduce an object registry #371

Are you sure you want to change the base?

Introduce an object registry #371

Conversation

georgebrock commented Aug 3, 2019

What?

Why?

Code review

hound bot Aug 3, 2019

Choose a reason for hiding this comment

hound bot Aug 3, 2019

Choose a reason for hiding this comment

hound bot Aug 3, 2019

Choose a reason for hiding this comment

hound bot Aug 3, 2019

Choose a reason for hiding this comment

hound bot Aug 3, 2019

Choose a reason for hiding this comment

hound bot Aug 3, 2019

Choose a reason for hiding this comment

hound bot Aug 3, 2019

Choose a reason for hiding this comment

hound bot Aug 3, 2019

Choose a reason for hiding this comment

hound bot Aug 3, 2019

Choose a reason for hiding this comment

hound bot Aug 3, 2019

Choose a reason for hiding this comment