Skip to content

Commit

Permalink
Merge pull request #241 from rgalonso/fix/handle-todo-comment-suffix
Browse files Browse the repository at this point in the history
fix: don't include source with issue URL comment
  • Loading branch information
alstr authored Nov 12, 2024
2 parents ff0c64d + 948c3e0 commit 59c6b53
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 15 deletions.
3 changes: 2 additions & 1 deletion Issue.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ class Issue(object):
"""Basic Issue model for collecting the necessary info to send to GitHub."""

def __init__(self, title, labels, assignees, milestone, body, hunk, file_name,
start_line, num_lines, markdown_language, status, identifier, identifier_actual, ref, issue_url, issue_number, start_line_within_hunk=1):
start_line, num_lines, prefix, markdown_language, status, identifier, identifier_actual, ref, issue_url, issue_number, start_line_within_hunk=1):
self.title = title
self.labels = labels
self.assignees = assignees
Expand All @@ -12,6 +12,7 @@ def __init__(self, title, labels, assignees, milestone, body, hunk, file_name,
self.file_name = file_name
self.start_line = start_line
self.num_lines = num_lines
self.prefix = prefix
self.markdown_language = markdown_language
self.status = status
self.identifier = identifier
Expand Down
28 changes: 19 additions & 9 deletions TodoParser.py
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ def _extract_issue_if_exists(self, comment_block, marker, hunk_info):
for line_number_within_comment_block, line in enumerate(comment_lines):
line_status, committed_line = self._get_line_status(line)
line_statuses.append(line_status)
cleaned_line = self._clean_line(committed_line, marker)
cleaned_line, pre_marker_length, post_marker_length = self._clean_line(committed_line, marker)
line_title, ref, identifier, identifier_actual = self._get_title(cleaned_line)
if line_title:
if prev_line_title and line_status == line_statuses[-2]:
Expand All @@ -423,6 +423,7 @@ def _extract_issue_if_exists(self, comment_block, marker, hunk_info):
+ comment_block['start'] + line_number_within_comment_block),
start_line_within_hunk=comment_block['start'] + line_number_within_comment_block + 1,
num_lines=1,
prefix=(' '*pre_marker_length)+(marker['pattern'] if marker['type'] == 'line' else '')+(' '*post_marker_length),
markdown_language=hunk_info['markdown_language'],
status=line_status,
identifier=identifier,
Expand Down Expand Up @@ -532,19 +533,28 @@ def _get_line_status(self, comment):
@staticmethod
def _clean_line(comment, marker):
"""Remove unwanted symbols and whitespace."""
comment = comment.strip()
post_marker_length = 0
if marker['type'] == 'block':
original_comment = comment
comment = comment.strip()
start_pattern = r'^' + marker['pattern']['start']
end_pattern = marker['pattern']['end'] + r'$'
comment = re.sub(start_pattern, '', comment)
comment = re.sub(end_pattern, '', comment)
# Some block comments might have an asterisk on each line.
if '*' in start_pattern and comment.startswith('*'):
comment = comment.lstrip('*')
comment = comment.strip()
pre_marker_length = original_comment.find(comment)
else:
pattern = r'^' + marker['pattern']
comment = re.sub(pattern, '', comment)
return comment.strip()
comment_segments = re.search(fr'^(.*?)({marker["pattern"]})(\s*)(.*)', comment)
if comment_segments:
pre_marker_text, _, post_marker_whitespace, comment = comment_segments.groups()
pre_marker_length = len(pre_marker_text)
post_marker_length = len(post_marker_whitespace)
else:
pre_marker_length = 0
return comment, pre_marker_length, post_marker_length

def _get_title(self, comment):
"""Check the passed comment for a new issue title (and reference, if specified)."""
Expand All @@ -553,13 +563,13 @@ def _get_title(self, comment):
title_identifier_actual = None
title_identifier = None
for identifier in self.identifiers:
title_pattern = re.compile(fr'(\b{re.escape(identifier)}\b)(\(([^)]+)\))?\s*:?\s*(.+)', re.IGNORECASE)
title_pattern = re.compile(fr'(^.*?)(\s*?)(\b{re.escape(identifier)}\b)(\(([^)]+)\))?\s*:?\s*(.+)', re.IGNORECASE)
title_search = title_pattern.search(comment)
if title_search:
title_identifier_actual = title_search.group(1)
title_identifier_actual = title_search.group(3)
title_identifier = identifier
ref = title_search.group(3) # may be empty, which is OK
title = title_search.group(4)
ref = title_search.group(5) # may be empty, which is OK
title = title_search.group(6)
break
return title, ref, title_identifier, title_identifier_actual

Expand Down
2 changes: 1 addition & 1 deletion main.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def process_diff(diff, client=Client(), insert_issue_urls=False, parser=TodoPars
old_line = file_lines[line_number]
remove = fr'(?i:{raw_issue.identifier}).*{re.escape(raw_issue.title)}'
insert = f'Issue URL: {client.get_issue_url(new_issue_number)}'
new_line = re.sub(remove, insert, old_line)
new_line = re.sub('^.*'+remove, raw_issue.prefix + insert, old_line)
# make sure the above operation worked as intended
if new_line != old_line:
# Check if the URL line already exists, if so abort.
Expand Down
4 changes: 0 additions & 4 deletions tests/test_process_diff.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,6 @@ def test_same_title_in_same_file(self):
self._setUp(['test_same_title_in_same_file.diff'])
self._standardTest(5)

# There is a known bug related to this issue, so until it's resolved
# this is an expected failure.
# See #229
@unittest.expectedFailure
def test_comment_suffix_after_source_line(self):
self._setUp(['test_comment_suffix_after_source_line.diff'])
self._standardTest(1)
Expand Down

0 comments on commit 59c6b53

Please sign in to comment.