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

Insert newlines if issue numbers would be inserted inside a code block #624

Merged
merged 8 commits into from
Nov 19, 2024
12 changes: 12 additions & 0 deletions src/towncrier/_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,17 @@ def render_fragments(
data: dict[str, dict[str, dict[str, list[str]]]] = {}
issues_by_category: dict[str, dict[str, list[str]]] = {}

def maybe_append_newlines(text: str) -> str:
# if a newsfragment text ends with a code block, we want to append two newlines
Copy link
Member

Choose a reason for hiding this comment

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

I think is best to have this comment as a docstring and explain why use case for this function.

and maybe move it outside of render_fragments.

The TODOs needs to be fixed before merge.
If they can't be fixed, we should have followup GitHub Issues created for each TODO and the comments should contain a link to the GitHub Issue


I know that we have get_indent that is embedded and has a comment instead of docstring... but I think (subjective opinion) that is a bad code practice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it outside of render_fragments, added docstring, and added a bunch of comments explaining the regex logic. Which is also very much changed because I realized it was horribly broken >.>

Personally I don't think embedded functions are a bad practice if the function is only intended as a helper for that one specific function and is exceedingly unlikely to be used by any other function, but having it outside is plenty fine. Mentioned it only being used by render_fragments in its docstring.

# so issue number(s) don't get inserted into the block
if re.search(r"::\n\n([ \t]*\S*)*$", text):
# We insert one space, the default template inserts another, which results
# in the correct indentation given default bullet indentation.
# TODO: This breaks on different indentation though, and would require instead
# doing this change after the template has been applied.... I thought?
return text + "\n\n "
return text

for section_name, section_value in fragments.items():
data[section_name] = {}
issues_by_category[section_name] = {}
Expand Down Expand Up @@ -381,6 +392,7 @@ def render_fragments(
# for the template, after formatting each issue number
categories = {}
for text, issues in entries:
text = maybe_append_newlines(text)
rendered = [render_issue(issue_format, i) for i in issues]
categories[text] = rendered

Expand Down
1 change: 1 addition & 0 deletions src/towncrier/newsfragments/614.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Multi-line newsfragments that ends with a code block will now have a newline inserted before appending the link to the issue, to avoid breaking formatting.
97 changes: 97 additions & 0 deletions src/towncrier/test/test_format.py
Original file line number Diff line number Diff line change
Expand Up @@ -462,3 +462,100 @@ def test_line_wrapping_disabled(self):
versiondata={"name": "MyProject", "version": "1.0", "date": "never"},
)
self.assertEqual(output, expected_output)

def test_trailing_block(self):
"""
Make sure a newline gets inserted before appending the issue number, if the
newsfragment ends with an indented block.
"""

fragments = {
"": {
(
"1",
"feature",
0,
): "text::\n\n def foo(): ..."
}
}
expected_output = """MyProject 1.0 (never)
=====================

Features
--------

- text::

def foo(): ...

(#1)


"""

# TODO: I copy-pasted the below lines from previous test, they probably contain
Copy link
Member

Choose a reason for hiding this comment

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

this needs to be cleanup before merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I .. don't think it contains irrelevant crap currently? But I'm not overly familiar with your test structure so it's very possible it still does.

# crap that's irrelevant to this test.
definitions = {
"feature": {"name": "Features", "showcontent": True},
}
template = read_pkg_resource("templates/default.rst")

fragments = split_fragments(fragments, definitions)
output = render_fragments(
template,
None,
fragments,
definitions,
["-", "~"],
wrap=True,
versiondata={"name": "MyProject", "version": "1.0", "date": "never"},
)
self.assertEqual(output, expected_output)

def test_trailing_block_nondefault_bullets(self):
"""
Test insertion of newlines after code block with single-file-no-bullets.
"""
# TODO: I expected this to break with the issue number being indented, but
# instead I got an extra newline? Which seems fine?

fragments = {
"": {
(
"1",
"feature",
0,
): "text::\n\n def foo(): ..."
}
}
expected_output = """MyProject 1.0 (never)
=====================

Features
--------

text::

def foo(): ...


(#1)

"""

definitions = {
"feature": {"name": "Features", "showcontent": True},
}
template = read_pkg_resource("templates/single-file-no-bullets.rst")

fragments = split_fragments(fragments, definitions)
output = render_fragments(
template,
None,
fragments,
definitions,
["-", "~"],
wrap=True,
versiondata={"name": "MyProject", "version": "1.0", "date": "never"},
)
self.assertEqual(output, expected_output)
Loading