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

services.Issue: directly access configuration #978

Merged

Conversation

ryneeverett
Copy link
Collaborator

@ryneeverett ryneeverett commented Mar 3, 2023

Based on #979, #976, and #973.

Rather than using the origin dictionary to pass configuration values
from Service to Issue, give Issue direct access to the config and
main_config objects.

There are several advantages to this approach:

  1. "Origin" is an unclear name of which a developer has to build a
    conception. It's elimination means the (more straightforward)
    conception of "config" can be transferred from the Service object.
    That's one fewer concept for developers to juggle, which is good for
    bugwarrior core maintenance and for service development.
  2. Elimination of get_service_metadata. This method's name also does
    not really convey it's purpose and I was unsurprised to find that
    many services pass data through this method which is never actually
    used by the Issue.
  3. Decoupling Service and Issue. Changes that only involve Issue
    and configuration no longer require meddling with Service, which
    is just a middle man.
  4. Elimination of bidirectional communication between Service base
    class and instances. The child classes were calling
    self.get_issue_for_record on the base class, which was calling
    self.get_service_metadata on the child class. In some ways this is
    a re-iteration of point (2) -- this is a pattern that leads to
    confusion and to me is a code smell.

The architectural trade-off is that Issue is now directly coupled to
configuration, but in my opinion the previous indirection was merely a
vicarious coupling that doesn't provide much benefit any more. When this
design was first implemented, configuration was parsed in the Service
class, so it made sense because it avoided duplication of parsing and
provided consistency; this is no longer a relevant consideration.

This is largely motivated by a desire to simplify services for #775.

Rather than using the `origin` dictionary to pass configuration values
from Service to Issue, give `Issue` direct access to the `config` and
`main_config` objects.

There are several advantages to this approach:

1. "Origin" is an unclear name of which a developer has to build a
   conception. It's elimination means the (more straightforward)
   conception of "config" can be transferred from the `Service` object.
   That's one fewer concept for developers to juggle, which is good for
   bugwarrior core maintenance and for service development.
2. Elimination of `get_service_metadata`. This method's name also does
   not really convey it's purpose and I was unsurprised to find that
   many services pass data through this method which is never actually
   used by the `Issue`.
3. Decoupling `Service` and `Issue`. Changes that only involve `Issue`
   and configuration no longer require meddling with `Service`, which
   is just a middle man.
4. Elimination of bidirectional communication between `Service` base
   class and instances. The child classes were calling
   `self.get_issue_for_record` on the base class, which was calling
   `self.get_service_metadata` on the child class. In some ways this is
   a re-iteration of point (2) -- this is a pattern that leads to
   confusion and to me is a code smell.

The architectural trade-off is that `Issue` is now directly coupled to
configuration, but in my opinion the previous indirection was merely a
vicarious coupling that doesn't provide much benefit any more. When this
design was first implemented, configuration was parsed in the `Service`
class, so it made sense because it avoided duplication of parsing and
provided consistency; this is no longer a relevant consideration.

This is largely motivated by a desire to simplify services for GothenburgBitFactory#775.
@ryneeverett ryneeverett merged commit 15ac6a2 into GothenburgBitFactory:develop Oct 13, 2023
24 checks passed
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.

1 participant