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

[jakkdl]

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

• Make sure changes are covered by existing or new tests.
• For at least one Python version, make sure local test run is green.
• Create a file in src/towncrier/newsfragments/. Describe your
  change and include important information. Your change will be included in the public release notes.
• Make sure all GitHub Actions checks are green (they are automatically checking all of the above).
• Ensure docs/tutorial.rst is still up-to-date. or wherever this behaviour should be documented

[adiroiban]

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.

[jakkdl]

Added newsfragment :)

[adiroiban]

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.

[adiroiban]

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

[jakkdl]

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.

[adiroiban]

this needs to be cleanup before merge.

[jakkdl]

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.

[jakkdl]

Ah, the reason that test_trailing_block_nondefault_bullets passed is because the single-file-no-bullets template already appends a newline before the issue being appended, which means that the trailing space being added by maybe_append_newlines (will be renamed to append_newlines_if_trailing_code_block) is ignored. So the test isn't especially useful and I'll remove it.

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.
It's theoretically possible that this is a regression for somebody if they already had custom handling of trailing codeblocks in their template, but I think that's exceedingly unlikely. What's more likely is that this fix isn't sufficient for somebody at some point and will require a new PR with an entirely different approach.

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

[adiroiban]

Hi John. Thanks for the updates.

Can you please look at updating the automated tests?

Right now, I care about RST format.
As long as there are no regression for RST format, and the existing Markdown tests pass, we can merge this.

I am not using towncrier with Markdown.

It looks like people want support for MD in towncrier.

So it's up to you and other people who want MD to define the requirements for Markdown and update the automated tests.

As long as the tests pass, I think that we can merge this.

Once this is release and people will start using it, I expect that we will receive more feedback

Thanks again

[jakkdl]

ugh, I accidentally deleted some spaces in 4a30295 but didn't notice it broke tests. Everything should be working now.

And yeah this won't detect or fix markdown code blocks, I'm leaving that for future contributors.

Side note: You might want to consider removing the requirement to wait for approval for first-time contributors to run workflows, as that slows down development considerably :)

[adiroiban]

Thanks for the update.

I have update the repo security rules to "Require approval for first-time contributors who are new to GitHub"

---

I don't have much time and energy to look into this issue.

The code looks good.

If the tests pass, we can merge it.

[adiroiban]

Looks good. Thanks!