-
Notifications
You must be signed in to change notification settings - Fork 122
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
base: trunk
Are you sure you want to change the base?
Conversation
Thanks for the PR. As long as the exiting automated tests pass, any change should be ok. This needs a newsfragment to be merged ;) I hope @SmileyChris will have some time to review this PR and check if he is happy with the solution. |
Added newsfragment :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current implementation is fine.
It needs a bit of cleanup before merge.
maybe_append_newlines
needs better documentation to explain what it tries to do.
src/towncrier/_builder.py
Outdated
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
src/towncrier/test/test_format.py
Outdated
|
||
""" | ||
|
||
# TODO: I copy-pasted the below lines from previous test, they probably contain |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Ah, the reason that This means that this fix might still mess up custom templates that changes indentation if they also have trailing code blocks, but I think they can work around it by conditionally appending a newline in the template. I suppose I could open an issue for that if you want, but it feels incredibly niche it's maybe better off waiting until somebody actually encounters it in the wild. edit: same goes for people embedding trailing tables/bullet lists/other RST formatting |
…Cleaned up and expanded test. Removed single-file-no-bullets test
for more information, see https://pre-commit.ci
Description
Fixes #614
I think the fix breaks on some non-default templates, I'm not sure how much of an issue that is - but if it's a blocker I can try to insert the newlines after rendering the template instead.
The tests can probably be cleaned up a bit, and have a few more cases inserted.
This only fixes code blocks, but e.g. tables would also break formatting. Leaving that for a future PR in case somebody cares about it though.
Checklist
src/towncrier/newsfragments/
. Describe yourchange and include important information. Your change will be included in the public release notes.
docs/tutorial.rst
is still up-to-date. or wherever this behaviour should be documented