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.

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

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

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. :)

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 :)

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. :(

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.

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

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

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

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.

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”. 😃