Skip to content

Commit

Permalink
services.Issue: directly access configuration
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ryneeverett committed Mar 20, 2023
1 parent 3d13e99 commit b391a33
Show file tree
Hide file tree
Showing 25 changed files with 108 additions and 205 deletions.
10 changes: 1 addition & 9 deletions bugwarrior/docs/contributing/new-service.rst
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ We will now implement an ``Issue`` class, which is essentially a wrapper for eac
def to_taskwarrior(self):
return {
'project': self.extra['project'],
'priority': self.origin['default_priority'],
'priority': self.config.default_priority,
'annotations': self.record.get('annotations', []),
'tags': self.get_tags(),
'entry': self.parse_date(self.record.get('createdAt')),
Expand Down Expand Up @@ -187,12 +187,6 @@ Now for the main service class which bugwarrior will invoke to fetch issues.
self.client = GitBugClient(path=self.config.path, port=self.config.port)
def get_service_metadata(self):
return {
'import_labels_as_tags': self.config.import_labels_as_tags,
'label_template': self.config.label_template,
}
def get_owner(self, issue):
# Issue assignment hasn't been implemented in upstream git-bug yet.
# See https://github.com/MichaelMure/git-bug/issues/112.
Expand All @@ -215,8 +209,6 @@ Now for the main service class which bugwarrior will invoke to fetch issues.
Here we see two required class attributes (pointing to the classes we previously defined) and two required methods.

The ``get_service_metadata`` method is not required, but can be used to expose additional data in the ``Issue.origin`` attribute.

The ``get_owner`` method takes an individual issue and returns the "assigned" user, so that bugwarrior can filter issues on this basis. In this case git-bug has not yet implemented this feature, but it generally will just involve returning a value from the ``issue`` dictionary.

The ``issues`` method is a generator which yields individual issue dictionaries.
Expand Down
46 changes: 15 additions & 31 deletions bugwarrior/services/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,22 +57,9 @@ def get_password(self, key, login='nousername'):
interactive=self.main_config.interactive)
return password

def get_service_metadata(self):
return {}

def get_issue_for_record(self, record, extra=None):
origin = {
'annotation_length': self.main_config.annotation_length,
'default_priority': self.config.default_priority,
'description_length': self.main_config.description_length,
'templates': self.config.templates,
'target': self.config.target,
'shorten': self.main_config.shorten,
'inline_links': self.main_config.inline_links,
'add_tags': self.config.add_tags,
}
origin.update(self.get_service_metadata())
return self.ISSUE_CLASS(record, origin=origin, extra=extra)
return self.ISSUE_CLASS(
record, self.config, self.main_config, extra=extra)

def build_annotations(self, annotations, url=None):
final = []
Expand Down Expand Up @@ -176,9 +163,10 @@ class Issue(abc.ABC):
# system and the string values 'H', 'M' or 'L'.
PRIORITY_MAP = {}

def __init__(self, foreign_record, origin=None, extra=None):
def __init__(self, foreign_record, config, main_config, extra=None):
self._foreign_record = foreign_record
self._origin = origin if origin else {}
self.config = config
self.main_config = main_config
self._extra = extra if extra else {}

def update_extra(self, extra):
Expand Down Expand Up @@ -209,11 +197,11 @@ def get_tags_from_labels(self,
template_variable='label'):
tags = []

if not self.origin[toggle_option]:
if not getattr(self.config, toggle_option):
return tags

context = self.record.copy()
label_template = Template(self.origin[template_option])
label_template = Template(getattr(self.config, template_option))

for label in labels:
normalized_label = re.sub(r'[^a-zA-Z0-9]', '_', label)
Expand All @@ -224,7 +212,7 @@ def get_tags_from_labels(self,

def get_added_tags(self):
added_tags = []
for tag in self.origin['add_tags']:
for tag in self.config.add_tags:
tag = Template(tag).render(self.get_template_context())
if tag:
added_tags.append(tag)
Expand All @@ -246,7 +234,7 @@ def get_taskwarrior_record(self, refined=True):
def get_priority(self):
return self.PRIORITY_MAP.get(
self.record.get('priority'),
self.origin['default_priority']
self.config.default_priority
)

def get_processed_url(self, url):
Expand All @@ -259,7 +247,7 @@ def get_processed_url(self, url):
returns a shortened URL; otherwise returns the URL unaltered.
"""
if self.origin['shorten']:
if self.main_config.shorten:
return URLShortener().shorten(url)
return url

Expand Down Expand Up @@ -295,8 +283,8 @@ def build_default_description(
'subtask': 'Subtask #',
}
url_separator = ' .. '
url = url if self.origin['inline_links'] else ''
desc_len = self.origin['description_length']
url = url if self.main_config.inline_links else ''
desc_len = self.main_config.description_length
return "%s%s#%s - %s%s%s" % (
MARKUP,
cls_markup.get(cls, cls.title()),
Expand All @@ -318,8 +306,8 @@ def get_template_context(self):

def refine_record(self, record):
for field in Task.FIELDS.keys():
if field in self.origin['templates']:
template = Template(self.origin['templates'][field])
if field in self.config.templates:
template = Template(self.config.templates[field])
record[field] = template.render(self.get_template_context())
elif hasattr(self, 'get_default_%s' % field):
record[field] = getattr(self, 'get_default_%s' % field)()
Expand Down Expand Up @@ -376,13 +364,9 @@ def record(self):
def extra(self):
return self._extra

@property
def origin(self):
return self._origin

def __str__(self):
return '%s: %s' % (
self.origin['target'],
self.config.target,
self.get_taskwarrior_record()['description']
)

Expand Down
6 changes: 3 additions & 3 deletions bugwarrior/services/azuredevops.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,9 @@ class AzureDevopsIssue(Issue):

def get_priority(self):
value = self.record.get("fields").get(
"Microsoft.VSTS.Common.Priority", self.origin['default_priority']
"Microsoft.VSTS.Common.Priority", self.config.default_priority
)
return self.PRIORITY_MAP.get(value, self.origin['default_priority'])
return self.PRIORITY_MAP.get(value, self.config.default_priority)

def to_taskwarrior(self):
return {
Expand All @@ -169,7 +169,7 @@ def to_taskwarrior(self):
self.STATE: self.record["fields"]["System.State"],
self.ACTIVITY: self.record.get("fields").get("System.Activity", ""),
self.PRIORITY: self.record.get("fields").get(
"Microsoft.VSTS.Common.Priority", self.origin['default_priority']
"Microsoft.VSTS.Common.Priority", self.config.default_priority
),
self.REMAINING_WORK: self.record.get("fields").get(
"Microsoft.VSTS.Scheduling.RemainingWork"
Expand Down
2 changes: 1 addition & 1 deletion bugwarrior/services/bts.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ def get_default_description(self):
def get_priority(self):
return self.PRIORITY_MAP.get(
self.record.get('severity'),
self.origin['default_priority']
self.config.default_priority
)


Expand Down
7 changes: 0 additions & 7 deletions bugwarrior/services/deck.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,13 +163,6 @@ def __init__(self, *args, **kwargs):
password=self.config.password
)

def get_service_metadata(self):
return {
'import_labels_as_tags': self.config.import_labels_as_tags,
'label_template': self.config.label_template,
'only_if_assigned': self.config.only_if_assigned,
}

def get_owner(self, issue):
return issue[issue.ASSIGNEE]

Expand Down
7 changes: 1 addition & 6 deletions bugwarrior/services/gerrit.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def to_taskwarrior(self):
'annotations': self.extra['annotations'],
self.URL: self.extra['url'],

'priority': self.origin['default_priority'],
'priority': self.config.default_priority,
'tags': [],
self.FOREIGN_ID: self.record['_number'],
self.SUMMARY: self.record['subject'],
Expand Down Expand Up @@ -104,11 +104,6 @@ def __init__(self, *args, **kw):
def get_keyring_service(config):
return f"gerrit://{config.base_uri}"

def get_service_metadata(self):
return {
'url': self.config.base_uri,
}

def get_owner(self, issue):
# TODO
raise NotImplementedError(
Expand Down
10 changes: 2 additions & 8 deletions bugwarrior/services/gitbug.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ class GitBugIssue(Issue):

def to_taskwarrior(self):
return {
'project': self.origin['target'],
'priority': self.origin['default_priority'],
'project': self.config.target,
'priority': self.config.default_priority,
'annotations': self.record.get('annotations', []),
'tags': self.get_tags(),
'entry': self.parse_date(self.record.get('createdAt')),
Expand Down Expand Up @@ -158,12 +158,6 @@ def __init__(self, *args, **kwargs):
port=self.config.port,
annotation_comments=self.main_config.annotation_comments)

def get_service_metadata(self):
return {
'import_labels_as_tags': self.config.import_labels_as_tags,
'label_template': self.config.label_template,
}

def get_owner(self, issue):
# Issue assignment hasn't been implemented in upstream git-bug yet.
# See https://github.com/MichaelMure/git-bug/issues/112.
Expand Down
8 changes: 1 addition & 7 deletions bugwarrior/services/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ def to_taskwarrior(self):

return {
'project': self.extra['project'],
'priority': self.origin['default_priority'],
'priority': self.config.default_priority,
'annotations': self.extra.get('annotations', []),
'tags': self.get_tags(),
'entry': created,
Expand Down Expand Up @@ -323,12 +323,6 @@ def __init__(self, *args, **kw):
def get_keyring_service(config):
return f"github://{config.login}@{config.host}/{config.username}"

def get_service_metadata(self):
return {
'import_labels_as_tags': self.config.import_labels_as_tags,
'label_template': self.config.label_template,
}

def get_owned_repo_issues(self, tag):
""" Grab all the issues """
issues = {}
Expand Down
17 changes: 4 additions & 13 deletions bugwarrior/services/gitlab.py
Original file line number Diff line number Diff line change
Expand Up @@ -401,12 +401,12 @@ class GitlabIssue(Issue):
# Override the method from parent class
def get_priority(self):
default_priority_map = {
'todo': self.origin['default_todo_priority'],
'merge_request': self.origin['default_mr_priority'],
'issue': self.origin['default_issue_priority']}
'todo': self.config.default_todo_priority,
'merge_request': self.config.default_mr_priority,
'issue': self.config.default_issue_priority}

type_str = self.extra['type']
default_priority = self.origin['default_priority']
default_priority = self.config.default_priority

return default_priority_map.get(type_str, default_priority)

Expand Down Expand Up @@ -515,15 +515,6 @@ def __init__(self, *args, **kw):
def get_keyring_service(config):
return f"gitlab://{config.login}@{config.host}"

def get_service_metadata(self):
return {
'import_labels_as_tags': self.config.import_labels_as_tags,
'label_template': self.config.label_template,
'default_issue_priority': self.config.default_issue_priority,
'default_todo_priority': self.config.default_todo_priority,
'default_mr_priority': self.config.default_mr_priority,
}

def get_owner(self, issue):
return [assignee['username'] for assignee in issue[1]['assignees']]

Expand Down
2 changes: 1 addition & 1 deletion bugwarrior/services/gmail.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ def to_taskwarrior(self):
'annotations': self.get_annotations(),
'entry': self.get_entry(),
'tags': [label for label in self.extra['labels'] if label not in self.EXCLUDE_LABELS],
'priority': self.origin['default_priority'],
'priority': self.config.default_priority,
self.THREAD_ID: self.record['id'],
self.SUBJECT: self.extra['subject'],
self.URL: self.extra['url'],
Expand Down
26 changes: 11 additions & 15 deletions bugwarrior/services/jira.py
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ def __get_sprints(self):
fields = self.record.get('fields', {})
sprints = sum((
fields.get(key) or []
for key in self.origin['sprint_field_names']
for key in self.extra['sprint_field_names']
), [])
for sprint in sprints:
if isinstance(sprint, dict):
Expand All @@ -305,7 +305,7 @@ def get_number(self):
return self.record['key'].rsplit('-', 1)[1]

def get_url(self):
return self.origin['url'] + '/browse/' + self.record['key']
return self.config.base_uri + '/browse/' + self.record['key']

def get_summary(self):
if self.extra.get('jira_version') == 4:
Expand All @@ -328,7 +328,7 @@ def get_priority(self):
value = str(value)
# priority.name format: "1 - Critical"
map_key = value.strip().split()[-1]
return self.PRIORITY_MAP.get(map_key, self.origin['default_priority'])
return self.PRIORITY_MAP.get(map_key, self.config.default_priority)

def get_default_description(self):
return self.build_default_description(
Expand Down Expand Up @@ -396,15 +396,14 @@ def __init__(self, *args, **kw):
},
**auth
)
self.import_sprints_as_tags = self.config.import_sprints_as_tags

self.sprint_field_names = []
if self.import_sprints_as_tags:
if self.config.import_sprints_as_tags:
field_names = [field for field in self.jira.fields()
if field['name'] == 'Sprint']
if len(field_names) < 1:
log.warn("No sprint custom field found. Ignoring sprints.")
self.import_sprints_as_tags = False
self.config.import_sprints_as_tags = False
else:
log.info("Found %i distinct sprint fields." % len(field_names))
self.sprint_field_names = [field['id']
Expand All @@ -414,15 +413,6 @@ def __init__(self, *args, **kw):
def get_keyring_service(config):
return f"jira://{config.username}@{config.base_uri}"

def get_service_metadata(self):
return {
'url': self.config.base_uri,
'import_labels_as_tags': self.config.import_labels_as_tags,
'import_sprints_as_tags': self.import_sprints_as_tags,
'sprint_field_names': self.sprint_field_names,
'label_template': self.config.label_template,
}

def get_owner(self, issue):
# TODO
raise NotImplementedError(
Expand All @@ -446,6 +436,12 @@ def annotations(self, issue, issue_obj):
issue_obj.get_processed_url(issue_obj.get_url())
)

def get_issue_for_record(self, record, extra=None):
if extra is None:
extra = {}
extra.setdefault('sprint_field_names', self.sprint_field_names)
return super().get_issue_for_record(record, extra=extra)

def issues(self):
cases = self.jira.search_issues(self.query, maxResults=None)

Expand Down
8 changes: 1 addition & 7 deletions bugwarrior/services/pagure.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def to_taskwarrior(self):
if self.extra['type'] == 'pull_request':
priority = 'H'
else:
priority = self.origin['default_priority']
priority = self.config.default_priority

return {
'project': self.extra['project'],
Expand Down Expand Up @@ -115,12 +115,6 @@ def __init__(self, *args, **kw):

self.session = requests.Session()

def get_service_metadata(self):
return {
'import_tags': self.config.import_tags,
'tag_template': self.config.tag_template,
}

def get_issues(self, repo, keys):
""" Grab all the issues """
key1, key2 = keys
Expand Down
Loading

0 comments on commit b391a33

Please sign in to comment.