New: Markdown Formatter #4040

EddyHaigh · 2020-09-27T20:57:39Z

Pull request checklist

Make sure you:

Signed the Contributor License Agreement
Followed the commit message guidelines

For non-trivial changes, please make sure you also:

Added/Updated related documentation.
Added/Updated related tests.

Short description of the change(s)

This change adds a Markdown formatter related to issue: #3962
The new formatter does add some duplication of code between the HTML and Markdown formatter and would benefit from creating a shared formatter utils package.

molant

Took a quick look between meetings and it's looking great.
Thanks a lot @EddyHaigh !

I left a couple comments but you might want to hold until @sarvaje and @antross can spend sometime before doing anything.

molant · 2020-09-28T17:26:06Z

packages/formatter-markdown/package.json

@@ -0,0 +1,86 @@
+{
+  "name": "@hint/formatter-markdown",
+  "version": "1.0.0",


I believe the first version needs to be 0.0.1 or similar so it gest bumped automatically by our release script.

molant · 2020-09-28T17:32:13Z

packages/formatter-markdown/src/formatter.ts

+    for (const lang of languagesToCheck) {
+        const file = path.join(rootPath, lang, messagesFileName);
+
+        if (fs.existsSync(file)) { // eslint-disable-line no-sync


Can you please add a comment saying fs.exists is deprecated and that's why we use the sync version?

molant · 2020-09-28T17:33:07Z

packages/formatter-markdown/src/formatter.ts

+     * @param result The webhint scan result.
+     */
+    /* istanbul ignore next [too hard to test] */
+    private createMarkdown(result: AnalysisResult) {


Can you add a comment with an example of how the markdown should look like?

molant · 2020-09-28T17:36:25Z

packages/formatter-markdown/src/formatter.ts

+        markdown += `${this.getMessage('hints')}: ${result.hintsCount}`;
+        markdown += MarkdownHelpers.newLine;
+
+        result.categories.forEach((category) => {


What do you think about refactoring this a bit to make it easier to read?
Maybe replace each forEach with a map so it can return the content for each one and then you can add them to markdown after a [].join(MardownHelpers.newlLine) or something similar?

@molant I've changed this to use map but I'm not sure if it is right or not.

molant · 2020-09-28T17:41:19Z

packages/formatter-markdown/package.json

+    "@hint/utils-types": "^1.1.0",
+    "lodash": "^4.17.20",
+    "fs-extra": "^9.0.1",
+    "moment": "^2.29.0"


This is a note for the core team:

We are using version 2.28.0 in another package (maybe that's why yarn.lock gets updated?)

moment is considered legacy now by their team, we should think about moving to other alternatives

Maybe we should create an issue to replace moment?

Done in #4044

I've seen we'have removed it from the HTML formatted so I've removed it from this formatter.

sarvaje

Looking good, just a few things.

sarvaje · 2020-09-29T15:54:51Z

packages/formatter-markdown/src/_locales/en/messages.json

@@ -0,0 +1,106 @@
+{
+    "backToTop": {


Can you clean this file, I think that most of the strings are not used.

sarvaje · 2020-09-29T16:18:10Z

packages/formatter-markdown/package.json

+    "@hint/utils-types": "^1.1.0",
+    "lodash": "^4.17.20",
+    "fs-extra": "^9.0.1",
+    "moment": "^2.29.0"


packages/formatter-markdown/src/formatter.ts

sarvaje · 2020-09-29T16:20:21Z

packages/formatter-markdown/src/formatter.ts

+import * as fs from 'fs-extra';
+
+import path = require('path');
+import { cwd } from 'process';


nit: Move not local import/requires to the top of the file (fs-extra, path, process)

sarvaje · 2020-09-29T16:46:28Z

packages/formatter-markdown/src/result.ts

@@ -0,0 +1,337 @@
+import * as path from 'path';


I see a lot of duplicated code with the formatter-html maybe it is worth it to open an issue to create a utils-formatter package to refactor both, formatter-html and formatter-markdown.

@webhintio/core thoughts?

sarvaje · 2020-09-29T16:59:22Z

packages/formatter-markdown/src/utils.ts

+     * @param level The header level e.g. h3 / ###.
+     */
+    public static createHeader(header: string, level: number): string {
+        return level > 0 ? ` ${header}`.padStart(1).padStart(header.length + 1 + level, '#') : header;


Is it padStart(1) necessary?

sarvaje

Thanks for the update!

Just a few more things :)

sarvaje · 2020-10-02T20:56:03Z

packages/formatter-markdown/markdown-webhint-report.md

@@ -0,0 +1,277 @@
+# Webhint Report - 2020-10-02T20:19:10.489Z


I don't think we need to have this as an example.

In my opinion, a shorter version (just one pass and one hint/warning/error) as part of the README.md would be enough.

@molant @antross what do you think?

packages/formatter-markdown/src/formatter.ts

sanmai-NL · 2020-11-20T10:47:59Z

@EddyHaigh Looking forward to this.

EddyHaigh requested review from antross, molant, sarvaje and utsavized as code owners September 27, 2020 20:57

molant reviewed Sep 28, 2020

View reviewed changes

molant mentioned this pull request Sep 29, 2020

Replace moment for maintained alternative #4044

Closed

sarvaje suggested changes Sep 29, 2020

View reviewed changes

sarvaje mentioned this pull request Oct 1, 2020

New: Added classList.add/remove leading '.' warning #4046

Merged

4 tasks

sarvaje suggested changes Oct 2, 2020

View reviewed changes

EddyHaigh force-pushed the feature/markdown-formatter branch from 676491f to fa764d3 Compare October 3, 2020 00:21

EddyHaigh requested a review from molant October 10, 2020 11:51

EddyHaigh force-pushed the feature/markdown-formatter branch 2 times, most recently from e54049e to 705b780 Compare November 12, 2020 00:17

antross added the agenda label Jan 5, 2021

New: Markdown Formatter

50764d4

EddyHaigh force-pushed the feature/markdown-formatter branch from a7fa443 to 50764d4 Compare January 21, 2021 14:16

EddyHaigh requested a review from sarvaje January 21, 2021 14:17

sarvaje force-pushed the main branch from 71d5154 to 4f450ac Compare February 4, 2021 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: Markdown Formatter #4040

New: Markdown Formatter #4040

EddyHaigh commented Sep 27, 2020

molant left a comment

molant Sep 28, 2020

molant Sep 28, 2020

molant Sep 28, 2020

molant Sep 28, 2020

EddyHaigh Oct 17, 2020

molant Sep 28, 2020

sarvaje Sep 29, 2020

molant Sep 29, 2020

sarvaje Sep 29, 2020

EddyHaigh Nov 12, 2020

sarvaje left a comment

sarvaje Sep 29, 2020

sarvaje Sep 29, 2020

sarvaje Sep 29, 2020

sarvaje Sep 29, 2020

sarvaje Sep 29, 2020

sarvaje left a comment

sarvaje Oct 2, 2020

sanmai-NL commented Nov 20, 2020

		@@ -0,0 +1,277 @@
		# Webhint Report - 2020-10-02T20:19:10.489Z

New: Markdown Formatter #4040

Are you sure you want to change the base?

New: Markdown Formatter #4040

Conversation

EddyHaigh commented Sep 27, 2020

Pull request checklist

Short description of the change(s)

molant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarvaje left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarvaje left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanmai-NL commented Nov 20, 2020