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

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

jakkdl
Copy link

@jakkdl jakkdl commented Jul 17, 2024

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

@jakkdl jakkdl requested a review from a team as a code owner July 17, 2024 15:19
@adiroiban
Copy link
Member

adiroiban commented Jul 17, 2024

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
Copy link
Author

jakkdl commented Aug 31, 2024

Added newsfragment :)

Copy link
Member

@adiroiban adiroiban left a 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.

@@ -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
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.


"""

# 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
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.

@jakkdl
Copy link
Author

jakkdl commented Sep 2, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues numbers added after rst preformatted blocks breaks rendering
2 participants