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

Add native markdown formatting for maven #1011

Merged
merged 10 commits into from Dec 23, 2021
Merged

Conversation

korthout
Copy link
Contributor

@korthout korthout commented Dec 2, 2021

This adds support for native formatting of markdown files, and enables it in Spotless for Maven.

Why?

So far, spotless only supported markdown formatting using the general formatting rules and through the use of prettier. However, the general formatting rules can only do so much. In addition, prettier requires npm and nodejs, which can be an unwanted dependency for maven users. By natively supporting markdown formatting, maven users can now avoid the npm/nodejs requirement, while having the benefit of comprehensive markdown formatting.

How?

This native support comes through the use of flexmark-java. A fork of commonmark-java that made it possible to parse many flavors of markdown into an AST and render it in some way. Among those renderers, there is the option to format to markdown. This renderer (or formatter) supports many formatting options. I've selected a few reasonable ones to get started, but they are not configurable yet.

This changeset adds a basic usage of flexmark-java as Markdown formatter. It adds a new <markdown> tag to the spotless maven plugin configuration, and subsequently adds a new <flexmark> tag a level deeper. Like all formatters it directly supports the default configurations (e.g. includes).

Usage

General usage looks like this:

<markdown>
  <flexmark />
<markdown>

By default it includes all *.md and **/*.md files.

Lastly, my thanks to everyone here. Spotless is a great tool. I've really been enjoying using it so far 🙌

Todo

  • add topic to CHANGES.md files
  • add documentation to README.md files

@korthout
Copy link
Contributor Author

korthout commented Dec 2, 2021

@nedtwigg This is not fully ready to be merged yet, but please have a look 🙂

@nedtwigg
Copy link
Member

nedtwigg commented Dec 3, 2021

This looks great! Just FYI, there is a way to write the glue code without doing a ton of reflection, I just pushed up #1012 as an example.

@korthout
Copy link
Contributor Author

@nedtwigg Wow, that was much easier 😆 Please have another look and let me know what the next step could be.

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. I made a few small nits, just needs docs and changelog entries.

Introduces a new step to natively format markdown. Previously markdown
was only available to spotless through the use of prettier which
requires nodejs. By natively supporting markdown, java users of spotless
have one less dependency to worry about.

This uses https://github.com/vsch/flexmark-java, a fork of
commonmark-java that made it possible to parse many flavors of markdown
into an AST and render it in some way. Among those renderers, there is
the option to format to markdown.

This renderer (or formatter) supports many formatting options. I've
selected a few reasonable ones to get started.

A test has been added which is inspired by the example from
https://github.com/vsch/flexmark-java/wiki/Markdown-Formatter. I've
extended it with some interesting things the formatter seems to take
care of.

During development, I had some difficulties dealing with idempotency of
the step. This is because the parser and formatter can be configured to
use different flavors individually. Care should be taken to apply the
same configurations to both the parser and formatter to keep it
idempotent.
Introduces a markdown section to the maven-plugin's configuration
parameters, which allows to specify the flexmark configuration.

Example:
```xml
<markdown>
  <flexmark />
</markdown>
```

This works exactly the same as the configuration of the other steps

By default all `.md` extented files are included, but as I understand
it, this can be overwritten using:

```xml
<markdown>
  <flexmark>
    <includes>...</includes>
  </flexmark>
</markdown>
```
This converts the flexmark step to use the compile-only flexmark glue
code, which makes using the library specific code much easier by
avoiding the java reflection api.
@korthout korthout force-pushed the flexmark-java branch 2 times, most recently from 0a45d90 to 48f3745 Compare December 17, 2021 13:52
@korthout korthout marked this pull request as ready for review December 17, 2021 14:49
@korthout
Copy link
Contributor Author

@nedtwigg Thanks for the comments, I've rebased on the default branch and made the requested changes. Please have another look.

plugin-maven/CHANGES.md Outdated Show resolved Hide resolved
@nedtwigg
Copy link
Member

Looks great! Just a few small changes (noted above) and we can ship it!

korthout and others added 4 commits December 18, 2021 12:37
None of these jars contain anything Eclipse related. They do however
contain the formatter and this might be interesting information.
Including '**/*.md' means that all md files in all sub directories are
found. In a multi-module maven project, where spotless is run for each
module it would mean that markdown files in sub modules are also found
by modules at a higher level of the module hierarchie (i.e. sub modules
are sub directories of an other module). This means that files would be
processed by the formatter multiple times. Which is obviously less
performant.

Generally, users should be proteced from this, by decent default
settings. However, if we would not include '**/*.md' users might be
surprised to learn that some of their files are left untouched.

Instead, I chose to warn multi-module users specifically about this.
Because the impact (and surprise) of not formatting files in nested
folder structures is likely greater than the hit in performance due to
processing the files multiple times. Also note that the first problem
occurs for both regular maven projects as well as multi-module projects
and the latter only occurs on multi-module projects.
@korthout
Copy link
Contributor Author

korthout commented Dec 18, 2021

I've made the requested changes and also thought long about the **/*.md default include. I think we should have it, but it presents a performance issue for multi-module maven projects, which I've described in e85c7ab.

In addition, I've expanded the maven-plugin readme documentation about this formatter. I imagine that the name flexmark does not make it clear to many users what this formatter does. So I hope this description helps most users.

@nedtwigg I think that this makes it fully ready for merging from my side. Please have another look.

@nedtwigg
Copy link
Member

I removed the default includes so that users are required to specify one manually. The performance of **/*.blah is much worse than src/**/*.blah, and there's no way for us to know in general where the markdown files will live. We require users to specify manually in other scenarios too (e.g. typescript). The previous default was reasonable too, but our issue tracker is full of people struggling with defaults that don't match their expectations, imo better to have no default at all for ambiguous cases.

This will get merged and released today.

@nedtwigg nedtwigg merged commit 376b561 into diffplug:main Dec 23, 2021
@nedtwigg
Copy link
Member

Published in plugin-maven 2.18.0.

@korthout
Copy link
Contributor Author

Thanks for the review @nedtwigg and changes. I agree with your conclusion on the includes. Happy to see it released 🚀

@korthout korthout deleted the flexmark-java branch December 23, 2021 21:21
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.

None yet

2 participants