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

changelog markdown lint issue #274

Closed
clarsonneur opened this issue Jan 21, 2019 · 8 comments · Fixed by #318
Closed

changelog markdown lint issue #274

clarsonneur opened this issue Jan 21, 2019 · 8 comments · Fixed by #318

Comments

@clarsonneur
Copy link

Hi,

This site is a great reference. We use it, so that anyone can understand what really changed.

But if I follow the current syntax, as a markdown document, my markdown lint reports some bad practices.
As an example, a changelog like as follow do not respect markdow linter:

# Changelog
All notable changes to this project will be documented in this file.

The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]
### Added
- security tools
- Make
### Changed
- Update to python3
- Helm v2.12.2
### Removed
### Fixed

## [1.0.1] - 2019-07-01
### Changed
- Kops v1.11.0

linter feedback:

In order to respect Markdown lint, I would propose such format:

# Changelog
All notable changes to this project will be documented in this file.

The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

- **Added**
  - security tools
  - Make

- **Changed**
  - Update to python3
  - Helm v2.12.2

## [1.0.1] - 2019-07-01

- **Changed**
  - Kops v1.11.0

For me it looks a little bit more readable, as well.

Feedback?

I can propose a PR with that kind of proposal.

@himedlooff
Copy link

Definitely prefer the line breaks, not so sure about moving from headings to nested lists. Why that change?

@olivierlacan
Copy link
Owner

Hey there @clarsonneur, I appreciate you bringing up this issue.

Headers in Markdown are often auto-linked, the choice to make groupings headers is purposeful as they are headers in a changelog. I'm not familiar with Markdown Lint but I also know that efforts to lint Markdown are often tricky considering there are myriad different competing Markdown implementations that don't follow the same rules or even offer the same syntax.

I'd be fine to consider this if we were producing broken output but we're not, this is a concern that you can solve by making an exception to your linter. Or perhaps contributing to the linter to fix the perfectly valid Markdown syntax we're using.

That said, have you tried using appropriate line breaks for your document. There should be no reasons to smoosh all these lines together, does this pass your linter:

## [Unreleased]

### Added
- security tools
- Make

### Changed
- Update to python3
- Helm v2.12.2

### Removed

### Fixed

## [1.0.1] - 2019-07-01

### Changed
- Kops v1.11.0

I know a lot of people find space and line breaks wasteful, but they often have meaning, especially in Markdown which is a paragraph-sensitive markup language that defaults to producing inline HTML elements when lines aren't separated by character returns.

See: https://daringfireball.net/projects/markdown/syntax#p

A simple example is the difference between these two Markdown chunks:

This
is
multiple
lines

Output:

This
is
multiple
lines

This 

is 

multiple

lines

Output:

This

is

multiple

lines


In GitHub-flavored Markdown, the output is multi-line in both cases. In many other forms of Markdown, they're not. There are also several Markdown implementations (or configurations if you use language-specific parsers) that prohibit header tangence (no empty line between the headers and the following content) with the rest of the content. I suspect this might be what you're dealing with here.

But even if it's not. I really don't want to bend around linter rules. If we were writing changelogs in a strict markup language with consistent rules across the board, I'd be with you. It would be silly to go against the flow. The trick with Markdown is that there is no standard despite many attempts at creating one.

With Keep a Changelog, I've made GitHub's Markdown parser my first target for output because in general a lot more people read the changelog Markdown output on GitHub than almost anywhere else. So you could say my linter is: does it look good on GitHub? And these days that extends to other major code hosting sites like GitLab, BitBucket, etc.

PS: Sorry for the lengthy response, please don't take this as a smackdown or the end of the discussion. It's common for maintainers like me to have a lot of unpublished thoughts and reasonings about why they picked something and avoided something else, and sometimes I catch a notification right after I've had my first coffee of the day. I promise it's nothing against you. 😃

@clarsonneur
Copy link
Author

hi @olivierlacan,

Thank you very much for your valuable feedback. And don't worry, I'm open minded, as maintainer of other projects as well... So I really appreciate your answer.

I do not have the whole picture of markdown linter used by others, as well.

Actually, I'm using VSC with the markdown-extension-pack plugin which comes with a linter which reported me lint warnings about missing line breaks just after headers.

I believe that VSC become so popular, so for that reason, I thought that would make sense to report and contribute to your project. But this is not necessarily a good argument.

have you tried using appropriate line breaks for your document. There should be no reasons to smoosh all these lines together, does this pass your linter:

## [Unreleased]
 
### Added
- security tools
- Make

### Changed
- Update to python3
- Helm v2.12.2

### Removed

### Fixed

  ## [1.0.1] - 2019-07-01

### Changed
- Kops v1.11.0

Yes, I did it. It is better than without line breaks. But no, it still reports me few warnings:

Line 1:

[markdownlint] MD002/first-heading-h1/first-header-h1: First heading should be a top level heading [Expected: h1; Actual: h2]
[markdownlint] MD041/first-line-h1: First line in file should be a top level heading

Line 3:

[markdownlint] MD022/blanks-around-headings/blanks-around-headers: Headings should be surrounded by blank lines

Line 4:

[markdownlint] MD032/blanks-around-lists: Lists should be surrounded by blank lines

etc ...

My proposal eliminates most of those warnings.

I have no trouble with line breaks in paragraph rendering and lint. So, it was really more related to line breaks after headers.


An answer to @himedlooff question:

not so sure about moving from headings to nested lists. Why that change?

Related to github markdown, I noticed also an issue when we set, multiple times, the same header, like changed, added.

[...]
### changed
[...]
### changed

Each headers are HTML labelled, so that we can create a link to the appropriate header.

If there is duplicated headers, label are duplicated as well but then not unique.
So we can't browse to any of those headers, except the first one.

That's why I got the lint warning as well
[markdownlint] MD024/no-duplicate-heading/no-duplicate-header: Multiple headings with the same content

And of course, if you create a TOC (like the automated TOC plugin), it becomes unreadable and useless.

@olivierlacan
Copy link
Owner

Each headers are HTML labelled, so that we can create a link to the appropriate header.

That's a good point. I wish GFM's header anchor generator dealt with that by creating unique identifiers and appending them to duplicate headings. There's nothing wrong with having headers named the same in HTML so I don't see why that should be a limitation of a markup language that creates HTML output. This is a pretty good example of the kinds of limitations we risk falling into if we try to abide by linters.

Semantically, Changed is a header. It's not a list item. You could argue that it's part of a list of notable changes itself for sure but I wouldn't format the document that way simply because it would result in many long and indigestible lists.

More importantly, a grouping header list item with no children list item will end up looking like a broken entry unless something like No notable changes is added as a child item:

## [Unreleased]

- **Changed**
  - Update to python3

- **Deprecated**

Compared to:

## [Unreleased]

### Changed

- Update to python3

### Deprecated

[markdownlint] MD002/first-heading-h1/first-header-h1: First heading should be a top level heading [Expected: h1; Actual: h2]

That's only because # Changelog is missing from the example.

I believe that VSC become so popular, so for that reason, I thought that would make sense to report and contribute to your project. But this is not necessarily a good argument.

It's a pretty good argument. 😄
I'd rather avoid having one the most popular editors in the world whine about our changelog format.

My proposal eliminates most of those warnings.

If I understand correctly, after the spacing changes I suggested and if you include the changelog example document headers, the only linting issue left is:

[markdownlint] MD024/no-duplicate-heading/no-duplicate-header: Multiple headings with the same content

That is a concern, I agree. We can definitely talk about it.

@clarsonneur
Copy link
Author

If I understand correctly, after the spacing changes I suggested and if you include the changelog example document headers, the only linting issue left is:

[markdownlint] MD024/no-duplicate-heading/no-duplicate-header: Multiple headings with the same content

That is a concern, I agree. We can definitely talk about it.

Yes. That's correct.

I'd rather avoid having one the most popular editors in the world whine about our changelog format.

I understand and agree.
I think what the linter suggested made sense to me and was not against the visual aspect of the document. So I accepted that kind of change. And then I proposed that to you.

FYI, In our context, we added the MD lint to the pipeline. So, that at least, we are quite sure that the document is well written from markdown perspective. The best for us, would be a changelog linter, but this is another subject.

@codykonior
Copy link

codykonior commented Apr 17, 2019

I also use VS Code and have this problem.

All that's required is:

  • Start with # Changelog
  • Have blank lines around header entries. This solves both headers following headers and lists following headers. The reason they state for this is as follows:

Rationale: Aside from aesthetic reasons, some parsers, including kramdown, will not parse headers that don't have a blank line before, and will parse them as regular text.

The last issue re: duplicate headings is fixed by creating a .markdownlint.json file in the root of your workspace or project with this content:

{
    "default": true,
    "MD024": {
        "siblings_only": true
    }
}

From then on the VS Code markdown lint knows to allow that.

@MOZGIII
Copy link
Contributor

MOZGIII commented Oct 20, 2019

This project should be fixed to just work with default, or contain proper instructions to configure linter accordingly.
Linters summarize de-facto standards for styling, and since this project is a public, community template is should follow the recommendations like everyone else (instead of arguing with the public standards, which would be the alternative of not obeying the linting rules, and what's happening currently). Having single unified, automatically enforced code style really simplifies life for all of use, but I assume that's well understood just because this project bases itself on a similar premise.

@MakisH
Copy link

MakisH commented Apr 17, 2021

The last issue re: duplicate headings is fixed by creating a .markdownlint.json file in the root of your workspace or project with this content:

{
    "default": true,
    "MD024": {
        "siblings_only": true
    }
}

Thank you for the suggestion! As in my project I wanted to keep the rule active, but make this exception only for CHANGELOG.md, a solution that I found to be cleaner was:

<!-- markdownlint-configure-file {"MD024": { "siblings_only": true } } -->

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.

6 participants