Skip to content

Conversation

@geeksam
Copy link
Contributor

@geeksam geeksam commented Apr 24, 2025

Description

Hi, Llewellyn! This is a followup from yesterday's pairing session. You didn't push your branch, but it was useful for me to engage with the code anyway. 😜

This moves us closer to being able to remove Launcher.

The solution

Similar to pairing session: creates a command method on [some] reporter classes that builds the actual command for that reporter. If we ignore all of the fancy metaprogramming, the only actual knowledge that Launcher still has at this point is the names of five reporter classes. (These are the ones that used to be singletons; I removed the Singleton implementation and gave them a temporary superclass of NamedReporter.)

First commit in branch adds characterization tests around the default_launcher methods for those five reporter classes; those tests can be removed when that method goes away.

There are still two distinct sets of reporters: those that inherit from Reporter and look like proper classes, and the five I've touched in this branch, which still look suspiciously like a glorified data structure (in that they only have one useful static method, and one inherited public instance method that only exists to satisfy the characterization tests that should go away. That's probably for a future pairing session.

Notation

Arlo's git notation

Summary by Sourcery

Refactor the Launcher module and reporter classes to simplify the implementation and move towards removing the Launcher module

Enhancements:

  • Replaced Singleton pattern with a new NamedReporter base class for specific reporter classes
  • Simplified command generation by moving command logic directly into reporter classes
  • Transformed Launcher module to use a more direct mapping of reporter classes

Tests:

  • Added new default_launcher_spec to verify existing launcher behavior
  • Updated existing launcher_spec to use quoted file paths

Chores:

  • Added characterization tests to support the refactoring
  • Prepared groundwork for future removal of Launcher module

@sourcery-ai
Copy link

sourcery-ai bot commented Apr 24, 2025

Reviewer's Guide by Sourcery

This pull request refactors the reporter classes to introduce a command method for building commands, removes the Singleton implementation, and updates the Launcher to use these new command methods. It also introduces a NamedReporter base class and adds characterization tests for the default_launcher methods.

Sequence diagram for reporter command execution

sequenceDiagram
  participant User
  participant Approvals
  participant NamedReporter
  participant Launcher
  participant DiffTool

  User->>Approvals: Verify(data, reporter)
  Approvals->>NamedReporter: report(received, approved)
  NamedReporter->>NamedReporter: default_launcher()
  NamedReporter->>Launcher: send(launcher_name)
  Launcher->>NamedReporter: command(received, approved)
  NamedReporter->>DiffTool: Execute command
  DiffTool-->>User: Show diff
Loading

Updated class diagram for Reporter hierarchy

classDiagram
  class Reporter
  class NamedReporter
  class DiffmergeReporter
  class FilelauncherReporter
  class OpendiffReporter
  class TortoisediffReporter
  class VimdiffReporter

  Reporter <|-- NamedReporter : Inherits from
  NamedReporter <|-- DiffmergeReporter : Inherits from
  NamedReporter <|-- FilelauncherReporter : Inherits from
  NamedReporter <|-- OpendiffReporter : Inherits from
  NamedReporter <|-- TortoisediffReporter : Inherits from
  NamedReporter <|-- VimdiffReporter : Inherits from

  NamedReporter : +default_launcher()
  DiffmergeReporter : +command(received, approved)
  FilelauncherReporter : +command(received, _)
  OpendiffReporter : +command(received, approved)
  TortoisediffReporter : +command(received, approved)
  VimdiffReporter : +command(received, approved)

  note for NamedReporter "Introduced NamedReporter class"
  note for DiffmergeReporter "Added command method"
  note for FilelauncherReporter "Added command method"
  note for OpendiffReporter "Added command method"
  note for TortoisediffReporter "Added command method"
  note for VimdiffReporter "Added command method"
Loading

File-Level Changes

Change Details Files
Introduce a command method to reporter classes, which constructs the command for the reporter.
  • The command method replaces the previous singleton pattern and default_launcher method.
  • The Singleton implementation was removed from reporter classes.
  • Reporter classes now inherit from a temporary NamedReporter class.
lib/approvals/reporters/diffmerge_reporter.rb
lib/approvals/reporters/filelauncher_reporter.rb
lib/approvals/reporters/opendiff_reporter.rb
lib/approvals/reporters/tortoisediff_reporter.rb
lib/approvals/reporters/vimdiff_reporter.rb
Update Launcher to return a lambda that, when invoked, returns the command by calling the reporter's command method.
  • The REPORTERS constant is now a hash mapping reporter names to their classes.
  • The launcher methods now return a lambda that calls the reporter's command method with the received and approved files.
  • The reporter-specific command methods (e.g., opendiff_command, diffmerge_command) were removed.
lib/approvals/reporters/launcher.rb
Introduce a NamedReporter base class to provide a default default_launcher method.
  • The NamedReporter class includes a default_launcher method that retrieves the appropriate launcher from the Launcher module.
  • The launcher_name method is used to determine the correct launcher name based on the class name.
lib/approvals/reporters/named_reporter.rb
Add characterization tests for the default_launcher methods on the reporter classes.
  • These tests verify that the default_launcher method returns the correct launcher from the Launcher module for each reporter.
spec/reporters/default_launcher_spec.rb
Update the launcher specs to assert that the commands include quotes around the file names.
  • The expected command strings in the launcher specs now include quotes around the received and approved file names.
spec/reporters/launcher_spec.rb

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

We've reviewed this pull request using the Sourcery rules engine. If you would also like our AI-powered code review then let us know.

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.

2 participants