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

Markdown lines should not have '# ' prepended #648

Closed
speedenator opened this issue Aug 4, 2024 · 18 comments · Fixed by #651
Closed

Markdown lines should not have '# ' prepended #648

speedenator opened this issue Aug 4, 2024 · 18 comments · Fixed by #651

Comments

@speedenator
Copy link

speedenator commented Aug 4, 2024

if using Markdown, each title has a '# ' prepended (build.py). However, this makes it a top-line header, and the example creates titles like:

# ## <rest of title>

as opposed of:

## <rest of title>

Certainly a few ways to fix this, such as a config like "default_md_prefix" which would be "# " and could be overridden to "" to ensure backwards compatibility. Happy to send a PR.

@speedenator
Copy link
Author

build.py line 235 btw

@adiroiban
Copy link
Member

for reference

if config.title_format:
top_line = config.title_format.format(
name=project_name, version=project_version, project_date=project_date
)
if is_markdown:
parts = [f"# {top_line}"]
else:
parts = [top_line, config.underlines[0] * len(top_line)]
parts.append(rendered)
content = "\n".join(parts)

@adiroiban
Copy link
Member

Thanks for the report

I am not very familiar to how people want to use towncrier with Markdown.

I am only using towncrier with reStructuredText.

I can see that this change was introduced here #610


Isn't this a bug ?

Why would you want this generated: # ## some title ?

Do we need to ensure backward compatiblity ?

@speedenator
Copy link
Author

Thanks for the quick reply.

I'm relatively new to towncrier so didn't want to pre-supposed a solution.

Given how recent the change is and how nobody else has piped up for a pretty obvious bug - I think the right short-term solution is just to get rid of '# ' in that f-string if it's a recent addition and few people (if any) use it. Worse case is that people who expect a Heading 1 (title) formatting can just put "# " in their title_string. This also has the benefit that you aren't putting markdown-specific encodings into the code.

@speedenator
Copy link
Author

@SmileyChris as the author - any comments re: above?

@adiroiban
Copy link
Member

Well.

By reading the PR, I can see that the # is there to allow the same title to be valid for both RST and MD formats... so this looks like a feature.

Can you please provide your full configuration so that we can review what can be done ?


If you need to have the titles as sub-sections, maybe we can have this logic for MD format:

  • If the custom format title does not start with #, prefix the title with #
  • Otherside, don't add a prefix.

I am not sure how valid is this logic ... just brainstorming on Sunday night :p

@speedenator
Copy link
Author

Fundamentally, having one format won't work given how the code is now. Markdown uses '#' ➝ '######' as a preface for various header levels, while rst uses underlines. My config is per below. A few things:

  • To use rst, I'd need to put something in underlines (or just get rid of the statement and use the default rst). But having blanks for underlines clearly won't work for rst.
  • title_format has '## ' as the prefix, for 2nd level titles. This is actually what's in the doc. URL format is also different - text link vs text link<url>.
  • start string comment is also different
[tool.towncrier]
name = "WZ"
directory = "changelog.d"
filename = "CHANGELOG.md"
# start_string = ".. towncrier release notes start"
start_string = "<!-- towncrier release notes start -->\n"
underlines = ["", "", ""]
title_format = "## [{version}](https://github.com/speedenator/wz/tree/{version}) - {project_date}"
issue_format = "[#{issue}](https://github.com/speedenator/wz/issues/{issue})"

I'd posit that the quick solution again is just to remove '# ' on that line. I would actually recommend avoiding going for a "one line for any format" kinda model... while there are a lot of similarities with rst & md, there are enough differences that things will get obnoxious quickly, like links. Better to let the user define it with whatever template they are using, or just don't support md and tell all us md users to either hop on the rst bus or use our favorite rst_to_md tool. :)

Note: the only other place I see markdown is for the issue type headers, which are correctly (IMHO) third-level and using three '###'s, but those look like they are in the template.md file (which overall seems OK, although I'm trying to figure out how title_format from above gets inserted as it clearly does in my setup but I don't see exactly how).

@adiroiban
Copy link
Member

Thanks.
I agree that towncrier build code should generate as little markup as possible.

The markup should be left to the title_format and the template file.

I agree that removing the # prefix makes sense.

I have approved the previous PR as I don't care that much about Markdown , and this was something that was ok for a Markdown user.

If you have time, please consider sending a PR to remove the code and update the tests.

Maybe extend the current markdown test to improve coverage for various configurations.

Thanks

@znichollscr
Copy link
Contributor

Just to jump in. Firstly, thanks for the package, it is amazing.

As another markdown user, #610 was a (slightly) painful regression. I hope to find time to make a PR to fix this. If you want someone to ping to review markdown related PRs, I'd be happy to volunteer.

@adiroiban
Copy link
Member

I will try to ping you for MD related reviews.

The imporant part is to make sure we have a good set of tests for all kind of Markdown usecase.

The problem is that towncriew was started with RST only support, and it was initially created only to support the specific needs for the twisted/twisted repo.

So, I think that the main problem with MD support, is the lack of a good set of tests.

znichollscr added a commit to climate-resource/input4mips_validation that referenced this issue Aug 5, 2024
While we wait for a fix to twisted/towncrier#648
@znichollscr
Copy link
Contributor

So, I think that the main problem with MD support, is the lack of a good set of tests.

Fair, will see what I can do

@znichollscr
Copy link
Contributor

Fix is in #651. I think it needs a maintainer to approve the workflow run?

@SmileyChris
Copy link
Contributor

Sorry for the lethargic response, but yes, it looks like my fix in #610 was definitely a bug. Removing the underlines made sense, but assuming the "#" was silly.

PS @adiroiban -- using the markdown doc page to determine what should and shouldn't be done isn't the best course of action. In reality, I'd prefer that page be deprecated or rewritten because unless you specifically care about the specific "Keep a Changelog" standardazation then you can now use towncrier for markdown without any of the steps required on that page.

@adiroiban
Copy link
Member

Thanks for the feedback.
I am not using markdown for any of my projects.

So my only reference for MD is the current documentation and tests.

I think that is up to the people who use markdown to make sure that the towncrier tests and documentation are ok

@ilan-gold
Copy link

ilan-gold commented Aug 22, 2024

Is there interest in this as well for the tool.towncrier.fragment customization? It would be great to be able to coordinate the two i.e., custom title and then subsection size handling. The title stuff works great, just tried out main with the fix

@adiroiban
Copy link
Member

Hi Ilan. Sorry I don't understand your request.

If there is anything that you would like to see in towncrier feel free to send a PR.
As long as it is documented and has automated tests, we should be able to merge it.

@ilan-gold
Copy link

@adiroiban What I mean is that if you do this same thing with each individual fragment title format, you end up with the extra space as in the full title when using markdown i.e.,

[tool.towncrier.fragment.performance]
name = "### Performance"

produces # ### Performance as the title

@adiroiban
Copy link
Member

Isn't this the issue fixed by #651 ?

Please try with 24.8.0 ... which was released now #660

zoni added a commit to zoni/obsidian-export that referenced this issue Aug 25, 2024
With twisted/towncrier#648 now fixed, we can
just rely on a correctly-set title_format again for properly formatted
markdown changelogs.
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 a pull request may close this issue.

5 participants