lint README and add gh workflow to do so #2784

naseemkullah · 2021-01-08T16:17:57Z

Signed-off-by: naseemkullah naseem@transit.app

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

mcollina · 2021-01-08T16:22:33Z

Thanks! This is actually nice.

docs/Encapsulation.md:22:1 MD004/ul-style Unordered list style [Expected: asterisk; Actual: dash]

naseemkullah · 2021-01-08T16:48:38Z

Thanks! This is actually nice.

docs/Encapsulation.md:22:1 MD004/ul-style Unordered list style [Expected: asterisk; Actual: dash]

My pleasure! Fixed, pardon the force push! :)

RafaelGSS

LGTM.

jsumners

I'm extremely hesitant to accept these automated changes. There are some significant formatting changes I have called out, and very likely many that I haven't.

CONTRIBUTING.md

PROJECT_CHARTER.md

README.md

docs/Ecosystem.md

docs/Errors.md

docs/Style-Guide.md

docs/Testing.md

docs/TypeScript.md

mcollina · 2021-01-09T09:50:02Z

Then let's not land this.

mcollina

I don't think these automated changes are good enough for us.

naseemkullah · 2021-01-09T13:04:16Z

In fact most thing @jsumners pointed it were manual errors on my end that the automation did not touch. I'll try again, this time only with automated changes and add any remaining broken rules to the ignored rules file.

naseemkullah · 2021-01-09T13:47:45Z

More rules have been ignored and a much lighter touch has been applied, PTAL.

.github/workflows/lint-md.yml

jsumners · 2021-01-10T00:43:54Z

Can we re-start this PR in a new one that addresses files in smaller, easier to digest, chunks? For example, maybe start off with the main README and one or two other documents. That would make it easier to review the rules and agree upon them before they are applied across everything. As we agree on the initial set of documents, a few more can be added, and so on until we have covered them all. I realize it will take more time, but this PR is really too much to review.

naseemkullah · 2021-01-10T01:10:19Z

Sure!

naseemkullah · 2021-01-11T12:29:03Z

PTAL @jsumners this PR has been restarted from scratch, only README is touched and most rules are turned off for the time being. Just blank lines and spaces. :)

.markdownlint.json

README.md

.markdownlint.yaml

jsumners

LGTM so far

RafaelGSS

It's a better approach, for sure. LGTM.

mcollina · 2021-01-12T08:56:28Z

Would you mind to add a script in package.json to run the markdown linting?

naseemkullah · 2021-01-12T13:02:56Z

Would you mind to add a script in package.json to run the markdown linting?

Good idea

naseemkullah · 2021-01-12T13:32:24Z

Would you mind to add a script in package.json to run the markdown linting?

done!

jsumners · 2021-01-12T13:42:06Z

package.json

@@ -20,7 +20,9 @@
    "test:ci": "npm run lint && npm run unit -- --cov --coverage-report=lcovonly && npm run test:typescript",
    "bench": "branchcmp -r 2 -g -s \"npm run benchmark\"",
    "benchmark": "npx concurrently -k -s first \"node ./examples/simple.js\" \"npx autocannon -c 100 -d 5 -p 10 localhost:3000/\"",
-    "license-checker": "license-checker --production --onlyAllow=\"MIT;ISC;BSD-3-Clause;BSD-2-Clause\""
+    "license-checker": "license-checker --production --onlyAllow=\"MIT;ISC;BSD-3-Clause;BSD-2-Clause\"",
+    "markdownlint": "markdownlint '**/*.md'",


This tool is really annoying. I can't find any documentation about what options it accepts in the configuration dotfile to determine if the target file list can be specified there instead of repeated throughout our code base.

As an example of what I'm looking for, https://node-tap.org/docs/configuring/ clearly shows how to generate a default configuration file. It also shows that --files can be added to a configuration like so:

files: - index.test.js - 'test/**/*.test.js'

Do you know if this same thing can be done with the Markdown lint tool?

I'm not sure, I'm sorry you are annoyed, I can close this PR, the purpose was to improve document quality but the amount of grief it seems to cause you does not warrant it.

I'm not against the PR. I'm just frustrated with the tool's lack of transparency. As best I can tell, markdownlint-cli is the thing that reads the configuration and kicks off the linter. But that tool's readme doesn't provide a list, or a link to a list, of all possible configuration options.

That's true.

I've filed:
igorshubovych/markdownlint-cli#158
igorshubovych/markdownlint-cli#159

maybe see what they say and can decide if the tool will be worth fastify maintainers' while.

I've ported to markdownlint-cli2 PTAL and let me know your preference!

The files we want to lint still need to be specified every time it is invoked though.

The openjs foundation uses this tool remark that seems more documented to lint MD files

DavidAnson · 2021-01-12T16:58:11Z

If you want to be paranoid, it may be desirable to explicitly list the enabled rules in the configuration as well so that new rules aren't applied unexpectedly.

Additionally, you may choose to set default:false and then list ONLY the enabled rules so that it's exceedingly clear what IS applied (vs. how it looks now listing what is NOT being applied).

I have also commented on both issues opened against the CLI. Thanks!

Signed-off-by: naseemkullah <naseem@transit.app> After discussing with project maintainers, majority of rules are ignored, though spacing is still improved. Only README is added for now. More markdown files can be added and rules turned on on a per-need basis.

Signed-off-by: naseemkullah <naseem@transit.app>

package.json

To see if fasitfy maintainers prefer this.

jsumners

What is the difference with these new changes?

naseemkullah · 2021-01-17T19:43:43Z

What is the difference with these new changes?

Not much in fact.

The new underlying CLI has more config options, none of which we use today but they are easy to find in the README.

Also thought I'd give it a try as the maintainer of both this CLI and it's underlying library (markdownlint) @DavidAnson has been very responsive to our feedback and may add a files config option in the future.

stale · 2021-06-26T03:27:15Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions · 2022-08-10T00:22:08Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

naseemkullah force-pushed the markdownlint branch from a84b735 to 76574d3 Compare January 8, 2021 16:47

RafaelGSS approved these changes Jan 8, 2021

View reviewed changes

jsumners requested changes Jan 9, 2021

View reviewed changes

mcollina requested changes Jan 9, 2021

View reviewed changes

naseemkullah requested review from jsumners and mcollina January 9, 2021 13:46

salmanm reviewed Jan 9, 2021

View reviewed changes

.github/workflows/lint-md.yml Outdated Show resolved Hide resolved

naseemkullah force-pushed the markdownlint branch from 03843c8 to 73bec6c Compare January 11, 2021 12:24

naseemkullah requested a review from salmanm January 11, 2021 12:29

naseemkullah changed the title ~~lint markdown and add gh workflow to do so~~ lint README and add gh workflow to do so Jan 11, 2021

naseemkullah force-pushed the markdownlint branch from 73bec6c to e91be7e Compare January 11, 2021 12:33

jsumners reviewed Jan 11, 2021

View reviewed changes

.markdownlint.json Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

naseemkullah requested a review from jsumners January 11, 2021 14:13

jsumners reviewed Jan 11, 2021

View reviewed changes

.markdownlint.yaml Outdated Show resolved Hide resolved

naseemkullah requested a review from jsumners January 11, 2021 15:48

jsumners approved these changes Jan 11, 2021

View reviewed changes

Eomm approved these changes Jan 11, 2021

View reviewed changes

Eomm added the documentation Improvements or additions to documentation label Jan 11, 2021

RafaelGSS approved these changes Jan 12, 2021

View reviewed changes

naseemkullah force-pushed the markdownlint branch from 321ba6e to 500669f Compare January 12, 2021 13:31

jsumners reviewed Jan 12, 2021

View reviewed changes

naseemkullah added 4 commits January 17, 2021 13:38

convert markdownlint to yaml and add comments

038fd3f

Signed-off-by: naseemkullah <naseem@transit.app>

add link to markdownlint rules doc

6b01aa7

Signed-off-by: naseemkullah <naseem@transit.app>

add markdownlint scripts

4747f0b

Signed-off-by: naseemkullah <naseem@transit.app>

naseemkullah force-pushed the markdownlint branch 2 times, most recently from fe185fb to 058c71f Compare January 17, 2021 18:43

DavidAnson reviewed Jan 17, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

naseemkullah force-pushed the markdownlint branch 3 times, most recently from 48e341a to 993a5ad Compare January 17, 2021 18:50

naseemkullah requested review from jsumners, RafaelGSS and Eomm January 17, 2021 18:50

try markdownlint-cli2

44cb56e

To see if fasitfy maintainers prefer this.

naseemkullah force-pushed the markdownlint branch from 993a5ad to 44cb56e Compare January 17, 2021 18:52

jsumners reviewed Jan 17, 2021

View reviewed changes

jsumners approved these changes Jan 18, 2021

View reviewed changes

Base automatically changed from master to main March 26, 2021 12:02

stale bot added the stale Issue or pr with more than 15 days of inactivity. label Jun 26, 2021

mcollina closed this Aug 9, 2021

github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lint README and add gh workflow to do so #2784

lint README and add gh workflow to do so #2784

naseemkullah commented Jan 8, 2021 •

edited

mcollina commented Jan 8, 2021

naseemkullah commented Jan 8, 2021

RafaelGSS left a comment

jsumners left a comment

mcollina commented Jan 9, 2021

mcollina left a comment

naseemkullah commented Jan 9, 2021

naseemkullah commented Jan 9, 2021

jsumners commented Jan 10, 2021

naseemkullah commented Jan 10, 2021

naseemkullah commented Jan 11, 2021

jsumners left a comment

RafaelGSS left a comment

mcollina commented Jan 12, 2021

naseemkullah commented Jan 12, 2021

naseemkullah commented Jan 12, 2021

jsumners Jan 12, 2021 •

edited

naseemkullah Jan 12, 2021

jsumners Jan 12, 2021

naseemkullah Jan 12, 2021

naseemkullah Jan 17, 2021

Eomm Jan 17, 2021

DavidAnson commented Jan 12, 2021

jsumners left a comment

naseemkullah commented Jan 17, 2021

stale bot commented Jun 26, 2021

github-actions bot commented Aug 10, 2022

lint README and add gh workflow to do so #2784

lint README and add gh workflow to do so #2784

Conversation

naseemkullah commented Jan 8, 2021 • edited

Checklist

mcollina commented Jan 8, 2021

naseemkullah commented Jan 8, 2021

RafaelGSS left a comment

Choose a reason for hiding this comment

jsumners left a comment

Choose a reason for hiding this comment

mcollina commented Jan 9, 2021

mcollina left a comment

Choose a reason for hiding this comment

naseemkullah commented Jan 9, 2021

naseemkullah commented Jan 9, 2021

jsumners commented Jan 10, 2021

naseemkullah commented Jan 10, 2021

naseemkullah commented Jan 11, 2021

jsumners left a comment

Choose a reason for hiding this comment

RafaelGSS left a comment

Choose a reason for hiding this comment

mcollina commented Jan 12, 2021

naseemkullah commented Jan 12, 2021

naseemkullah commented Jan 12, 2021

jsumners Jan 12, 2021 • edited

Choose a reason for hiding this comment

naseemkullah Jan 12, 2021

Choose a reason for hiding this comment

jsumners Jan 12, 2021

Choose a reason for hiding this comment

naseemkullah Jan 12, 2021

Choose a reason for hiding this comment

naseemkullah Jan 17, 2021

Choose a reason for hiding this comment

Eomm Jan 17, 2021

Choose a reason for hiding this comment

DavidAnson commented Jan 12, 2021

jsumners left a comment

Choose a reason for hiding this comment

naseemkullah commented Jan 17, 2021

stale bot commented Jun 26, 2021

github-actions bot commented Aug 10, 2022

naseemkullah commented Jan 8, 2021 •

edited

jsumners Jan 12, 2021 •

edited