Markdown output #439

SmileyChris · 2022-09-29T01:02:42Z

Description

Provide markdown compatible formatting.

Fixes #128.

Config values for underlines and title_prefixes are have different defaults when a filename is configured that has an .md file extension.

Oh, and it turns out that python's markdown implementation doesn't handle multi-line bullets unless they are indented and have an empty line between each, so it also renders them like that now.

New configuration options:

title_prefixes (default of ["", "", ""] unless in .md mode, then ["# ", "## ", "### "])
issues_spaced (default False unless in .md mode)
bullet (default of "- " unless in .md mode, then " - ")

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.

…on is `.md`

…when configured filename ends with '.md'

for more information, see https://pre-commit.ci

codecov · 2022-09-29T01:07:17Z

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (ea7d1a6) compared to base (cd2d434).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##             trunk      #439   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          600       610   +10     
  Branches       144       144           
=========================================
+ Hits           600       610   +10

Impacted Files	Coverage Δ
src/towncrier/build.py	`100.00% <ø> (ø)`
src/towncrier/_builder.py	`100.00% <100.00%> (ø)`
src/towncrier/_settings/load.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

for more information, see https://pre-commit.ci

…pty line between bullets to render multi-line bullets correctly

for more information, see https://pre-commit.ci

hynek · 2022-09-29T04:16:13Z

Oh, and it turns out that python's markdown implementation doesn't handle multi-line bullets unless they are indented and have an empty line between each, so it also renders them like that now.

Ooof markdown looks broken and I don't think we should care about that.

I couldn't make it work even with you changes:

In [11]: md.convert("""
    ...: # Test
    ...:
    ...:  - bullet 1
    ...:
    ...:    b1 p2
    ...:
    ...:  - bullet 2
    ...:
    ...:    b2 p2
    ...: """)
Out[11]: '<h1>Test</h1>\n<ul>\n<li>bullet 1</li>\n</ul>\n<p>b1 p2</p>\n<ul>\n<li>bullet 2</li>\n</ul>\n<p>b2 p2</p>'

In [12]: print(_)
<h1>Test</h1>
<ul>
<li>bullet 1</li>
</ul>
<p>b1 p2</p>
<ul>
<li>bullet 2</li>
</ul>
<p>b2 p2</p>

I also don't think we should care. Changelogs are mainly rendered by web interfaces, so if anything we should focus on CommonMark / GitHub. I would really not want such warts in our APIs. The much more modern markdown-it does it correctly:

In [1]: import markdown_it; md = markdown_it.MarkdownIt()

In [4]: print(md.render("""
   ...: # Test
   ...:
   ...: - b1 p1
   ...:
   ...:   b1 p2
   ...: - b2 p1
   ...:
   ...:   b2 p2
   ...:   """))
<h1>Test</h1>
<ul>
<li>
<p>b1 p1</p>
<p>b1 p2</p>
</li>
<li>
<p>b2 p1</p>
<p>b2 p2</p>
</li>
</ul>

So if we decide to move on with this, please don't encode weird behaviors into our defaults, Markdown is weird enough as it is. :)

SmileyChris · 2022-09-29T04:45:06Z

You couldn't get to work because you didn't lstrip, so it had leading spaces:

>>> from markdown import markdown
>>> print(markdown("""
... # Test
... 
...   - bullet 1
... 
...     b1 p2
... 
...   - bullet 2
... 
...     b2 p2
... """.strip()))
<h1>Test</h1>
<ul>
<li>
<p>bullet 1</p>
<p>b1 p2</p>
</li>
<li>
<p>bullet 2</p>
<p>b2 p2</p>
</li>
</ul>

I assumed since markdown is the common package people install in Python and this is a python package that maybe there'd be some caring... but you're probably right - it's just ugly!

It should be easy to just revert that since I did the "warty" fix in one commit :)

hynek · 2022-09-29T04:48:14Z

Looking closer at the PR, I'm not super excited about this; let me explain:

Fact is, you already can do Markdown with Towncrier.

What this PR does is making is more convenient / magic-ey – that's fair and entirely desirable!

The price though is that the implementation gets even more convoluted. Maybe if you revert the markdown kludges above I might change my view, but as someone who spent the past weeks trying to make towncrier more maintainable this looks like a step back. :(

adiroiban

I just did a quick look at this.

Are we using templates/default.rst to generate Markdown output?

I think is best to have a separate templates/default.md Jinja template.

if filename ends with '.md` we can auto-magically set the Mardown defaults.

If template is not defined in the config we use a default markdown specific templat.

So we leave the current rendering engine as it is, as just fiddle with the template and current configs

Would that work?

And many thanks for putting all this together :)

adiroiban · 2022-09-29T09:45:23Z

src/towncrier/_settings/load.py

@@ -178,8 +198,13 @@ def parse_toml(base_path: str, config: Mapping[str, Any]) -> Config:
        start_string=config.get("start_string", _start_string),
        title_format=config.get("title_format", _title_format),
        issue_format=config.get("issue_format"),
-        underlines=config.get("underlines", _underlines),
+        underlines=config.get("underlines", _md_underlines if is_md else _underlines),


I find it easier to read.

Suggested change

underlines=config.get("underlines", _md_underlines if is_md else _underlines),

underlines=underlines,

adiroiban · 2022-09-29T09:54:33Z

src/towncrier/_settings/load.py

@@ -164,11 +182,13 @@ def parse_toml(base_path: str, config: Mapping[str, Any]) -> Config:
            failing_option="template",
        )

+    filename = config.get("filename", "NEWS.rst")
+    is_md = filename.endswith(".md")


Can we have all the if_md values and default selection in a common block.

Is ok to even move this to a separate function like `defaults = _get_missing_defaults(config)"

Suggested change

is_md = filename.endswith(".md")

underlines = config.get("underlines", "")

title_prefixes=config.get("title_prefixes", "")

if filename.endswith(".md"):

# Try to use markdown default.

if no underlines:

underlliens = ["", "", ""]

if no underlines:

# Fallback to rst defaults.

underlines = ["=", "-", "~"]

And I hope we don't need md_prefixes and md_bullet, as we can handle all this in the custom tempalates/default.md

What do you think?

hynek · 2022-09-29T13:00:10Z

My suggestion would be at this point:

remove the workarounds around the markdown package weirdnesses
split the templates as Adi suggested
don’t screw with anything else yet.

We must decouple the logic, but first we need to have a common ground zero. Those nested conditionals were a nightmare before and we really ought not make them worse. This needs some bigger refactorings than fiddling with if statements if we don’t want to make matters worse.

SmileyChris · 2022-09-29T19:54:43Z

Let's think about this from what would be best and least convoluted then.

Provide a separate default.md template.
Make this the default template if a configured filename has an .md extension.

That seems pretty straight forward. How does this approach sound?

adiroiban · 2022-09-29T20:12:50Z

That seems pretty straight forward. How does this approach sound?

Sound good. Thanks!

As @hynek mentions, we should try to keep this code as simple as possible... otherwise we will end up with a lot of bugs.

There are not too many automated tests (even with 100% code coverage), and we have limited dev resources.

I am looking after this project always in a hurry, trying to help and not to block the development...but I don't have time to sit down and fully evaluate all the changes and think about the side effects

So, a code that is simple, is best.

hynek · 2022-09-30T04:20:17Z

JFTR: having tons of conditionals and prefixed variables is code screaming for polymorphism. I think it would be a strict positive to untangle towncrier from rST (internally for now) and would be happy to help you to do it as part of this PR if you’re willing to play along (or we split the work into this PR only providing a template with the detection and do the refactoring + more comfort in a next PR).

it’s the old “make it work, make it right,” and in this case “make it pretty”. 😃

SmileyChris added 3 commits September 29, 2022 12:31

Output to markdown compatible format when configured filename extensi…

0f2588e

…on is `.md`

Add test for different configured underlines/title_prefixes defaults …

3e6b620

…when configured filename ends with '.md'

Test markdown options against both draft and build modes

7f6e545

SmileyChris requested a review from a team as a code owner September 29, 2022 01:02

[pre-commit.ci] auto fixes from pre-commit.com hooks

8116d90

for more information, see https://pre-commit.ci

SmileyChris mentioned this pull request Sep 29, 2022

Add a Markdown how-to #436

Merged

SmileyChris and others added 7 commits September 29, 2022 14:17

Add newsfragment and docs

a0dcb22

Fix docs formatting typo

e6277d5

[pre-commit.ci] auto fixes from pre-commit.com hooks

729f925

for more information, see https://pre-commit.ci

Python's standard markdown implementation needs indentation and an em…

70f9364

…pty line between bullets to render multi-line bullets correctly

[pre-commit.ci] auto fixes from pre-commit.com hooks

7bacdcf

for more information, see https://pre-commit.ci

Fix test for windows

904d266

Try to fix windows test again

ea7d1a6

adiroiban reviewed Sep 29, 2022

View reviewed changes

SmileyChris mentioned this pull request Mar 1, 2023

Default markdown template #483

Merged

6 tasks

SmileyChris closed this Mar 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Markdown output #439

Markdown output #439

SmileyChris commented Sep 29, 2022 •

edited

codecov bot commented Sep 29, 2022 •

edited

hynek commented Sep 29, 2022

SmileyChris commented Sep 29, 2022

hynek commented Sep 29, 2022

adiroiban left a comment •

edited

adiroiban Sep 29, 2022

adiroiban Sep 29, 2022

hynek commented Sep 29, 2022

SmileyChris commented Sep 29, 2022

adiroiban commented Sep 29, 2022

hynek commented Sep 30, 2022

	underlines=config.get("underlines", _md_underlines if is_md else _underlines),
	underlines=underlines,

-    is_md = filename.endswith(".md")
+    underlines = config.get("underlines", "")
+    title_prefixes=config.get("title_prefixes", "")
+    if filename.endswith(".md"):
+        # Try to use markdown default.
+        if no underlines:
+            underlliens = ["", "", ""]
+        if no underlines:
+            # Fallback to rst defaults.
+            underlines = ["=", "-", "~"]

Markdown output #439

Markdown output #439

Conversation

SmileyChris commented Sep 29, 2022 • edited

Description

Checklist

codecov bot commented Sep 29, 2022 • edited

Codecov Report

hynek commented Sep 29, 2022

SmileyChris commented Sep 29, 2022

hynek commented Sep 29, 2022

adiroiban left a comment • edited

Choose a reason for hiding this comment

adiroiban Sep 29, 2022

Choose a reason for hiding this comment

adiroiban Sep 29, 2022

Choose a reason for hiding this comment

hynek commented Sep 29, 2022

SmileyChris commented Sep 29, 2022

adiroiban commented Sep 29, 2022

hynek commented Sep 30, 2022

SmileyChris commented Sep 29, 2022 •

edited

codecov bot commented Sep 29, 2022 •

edited

adiroiban left a comment •

edited