Run docs through markdown linter #1801

roji · 2019-10-02T14:20:10Z

Had a bit of an obsessive spurt today so went through all our docs with a linter.

Added .markdownlint.json to help keep things clean. For VS Code there's a great markdownlint plugin which creates warnings - highly recommended.

bricelam

Hate it. lol, but I'm willing to conform.

Let's confirm the rendering concerns before merging.

entity-framework/core/managing-schemas/migrations/history-table.md

entity-framework/core/managing-schemas/migrations/teams.md

entity-framework/core/get-started/install/index.md

bricelam · 2019-10-02T16:55:06Z

entity-framework/core/managing-schemas/migrations/index.md


 Install the [command-line tools](xref:core/miscellaneous/cli/index):
+


I never liked requiring a space before lists. I'm always glad when it's allowed

I do find it more readable - after all in actual rendered HTML there's also some spacing (it's like a different paragraph I think).

You can always turn off linting rules you don't like. See https://github.com/dotnet/docs/blob/master/.markdownlint.json for an example.
But I do prefer the new line before lists myself.

bricelam · 2019-10-02T16:56:25Z

entity-framework/core/managing-schemas/migrations/providers.md

@@ -74,6 +81,5 @@ if (migrationBuilder.ActiveProvider == "Microsoft.EntityFrameworkCore.SqlServer"
 }
 ```

-
  [1]: ../../miscellaneous/cli/index.md


I like to put two newlines before these to make them feel more like footnotes

MarkdownLinter says no :)

Just kidding, I don't really have strong feelings about most of these, but it seems better to just follow along, otherwise we start to get arbitrary spacing all around the place?

I'm second guessing using this style of links at all in our docs. They're optimized for reading the raw markdown, but can become tedious when editing it.

I fully agree, we should just inline links...

bricelam · 2019-10-02T16:57:37Z

entity-framework/core/miscellaneous/cli/powershell.md

@@ -143,8 +146,8 @@ Parameters:

 | Parameter                         | Description                                                                                                             |
 |:----------------------------------|:------------------------------------------------------------------------------------------------------------------------|
-| <nobr>-Name \<String><nobr>       | The name of the migration. This is a positional parameter and is required.                                              |
-| <nobr>-OutputDir \<String></nobr> | The directory (and sub-namespace) to use. Paths are relative to the target project directory. Defaults to "Migrations". |


These (<nobr>) were to help with rendering. Wrapping looks terrible

Can put back, although is obsolete (there's a CSS for that). Not sure if markdown has a better way of dealing with this. In any case, do you think it's really needed in the first column of this table? Also we don't do this systematically anywhere.

It was breaking after they hyphen on smaller screens. I think I tried the css alternative and it didn't work...

@bricelam @roji Was there a resolution here?

Absolutely no strong feelings here, will bring back <nobr> until we see some reason why it's not a good idea.

entity-framework/core/miscellaneous/rc2-rtm-upgrade.md

bricelam · 2019-10-02T16:59:59Z

entity-framework/core/modeling/inheritance.md


 [!code-csharp[Main](../../../samples/core/Modeling/Conventions/InheritanceDbSets.cs?highlight=3-4&name=Model)]

-If you don't want to expose a *DbSet<TEntity>* for one or more entities in the hierarchy, you can use the Fluent API to ensure they are included in the model.
+If you don't want to expose a `DbSet<TEntity>` for one or more entities in the hierarchy, you can use the Fluent API to ensure they are included in the model.


Backticks make it a lot more jarring

IIRC we can also escape it with backslash, I guess it's a separate question of how we refer to things. I'd go even farther and say that the generic adds nothing, we could just talk about DbSet in our documentation...

Ah, it was for the generic. Yeah *DbSet\<TEntity>* would also work. I'm fine with backticks, escaping, or using the non-generic form. You decide

I personally think we have too many jarring code fragments in our docs. I really preferred it if we just talked about contexts rather than DbContext<T> (or even DbContext or DbContext). For DbSet I'd go with the non-generic non-quoted version, and so forth.

But that seems like a decision to decide as a team.

@bricelam @roji Was a decision made here?

I guess it's a general docs question, not really lint-related - we're pretty inconsistent across our docs on this (the linter simply complained about the unescaped angle bracket). We can discuss in design.

Since this point is already pretty inconsistent across our docs, I can just change this back to bold (with backslash) and handle the general question in another issue.

If you want to be consistent with what we do for dotnet/docs, APIs are either inside backticks (to prevent them from being localized) or they use the xref links to link to the API docs.

entity-framework/core/providers/provider-log.md

.markdownlint.json

smitpatel

Rules should not be disabled unless we have a good justification why we want to deviate from it. The doc authoring pack comes from docs team so they are kind of guidelines for us.

bricelam · 2019-10-02T19:28:18Z

The doc authoring pack comes from docs team

But the linter didn't. Do other docs repos have a .markdownlint.json file? Is it more or less consistent?

bricelam · 2019-10-02T19:38:44Z

Is there a syntax for suppressing warnings?

<!-- #prama warning disable MD024 -->

smitpatel · 2019-10-02T20:02:48Z

@bricelam https://github.com/aspnet/AspNetCore.Docs/blob/master/.markdownlint.json

roji · 2019-10-02T21:07:10Z

It seems to be possible to suppress in specific places, but I loath the idea of starting to put these in our docs.

bricelam · 2019-10-02T21:25:48Z

Hmm, ASP.NET is pretty different than https://github.com/dotnet/docs/blob/master/.markdownlint.json

roji · 2019-10-02T21:29:04Z

Wow, I wasn't even aware all these other repos were doing markdown linter, felt kind of like a trailblazer 😢

roji · 2019-10-04T16:19:54Z

Removed suppression for MD024 (double heading).

Guys, what's the next step here? Is this worth having a full discussion on in design or whatever? I'd like to merge this soon to avoid conflicts etc., we can always do a second wave later and remove more suppressions.

bricelam · 2019-10-04T16:45:25Z

I'm fine merging. Ultimately, I only care about the rendering, not the markdown formatting.

smitpatel · 2019-10-04T17:07:11Z

I am also ok with merging as long as we don't suppress things. A wrong suppressions means valid issues can go unnoticed.

roji · 2019-10-04T19:38:34Z

I am also ok with merging as long as we don't suppress things. A wrong suppressions means valid issues can go unnoticed.

Can you concretely say what you want us to do with the remaining suppressions? I think I addressed the reasoning behind the remaining four suppressions.

Feedback incorporated.

mairaw · 2019-10-15T02:14:20Z

entity-framework/core/miscellaneous/cli/powershell.md

@@ -143,8 +146,8 @@ Parameters:

 | Parameter                         | Description                                                                                                             |
 |:----------------------------------|:------------------------------------------------------------------------------------------------------------------------|
-| <nobr>-Name \<String><nobr>       | The name of the migration. This is a positional parameter and is required.                                              |
-| <nobr>-OutputDir \<String></nobr> | The directory (and sub-namespace) to use. Paths are relative to the target project directory. Defaults to "Migrations". |
+| -Name \<String>                   | The name of the migration. This is a positional parameter and is required.                                              |


Wondering if adding the parameters inside backticks could help with the rendering problem or make it worse.

Suggested change

| -Name \<String> | The name of the migration. This is a positional parameter and is required. |

| `-Name <String>` | The name of the migration. This is a positional parameter and is required. |

Should probably remove the backslash in front of String if you're wrapping in backticks

FWIW I played around with this a bit... the backticks make the text stand out much more and is quite unnatural/jarring to me. Making the browser very narrow makes the second argument drop to a separate line (i.e. -Name\n<string>) but it seem really OK to me (all very subjective of course).

Unless someone objects (@bricelam?) I'd say we merge as-is (no backticks, no special non-breaking directive).

Indeed. Fixed suggestion.

It's up to you guys. On the contributor guide they also talk about some new table formats but I've never managed to use them and be successful on solving my rendering issues with them
But leaving here for FYI: https://review.docs.microsoft.com/en-us/help/contribute/markdown-reference?branch=master#tables

mairaw · 2019-10-15T02:18:29Z

If you look at the history of our file, you'll see that @nschonni was helping us with those, so you can still consider yourself a trailblazer @roji. 😄

nschonni

FYI, was looking at adding a small GitHub Action over on the dotnet repo to just check stuff as it came in

.markdownlint.json

nschonni · 2019-10-15T02:27:14Z

entity-framework/core/miscellaneous/cli/powershell.md

@@ -143,8 +146,8 @@ Parameters:

 | Parameter                         | Description                                                                                                             |
 |:----------------------------------|:------------------------------------------------------------------------------------------------------------------------|
-| <nobr>-Name \<String><nobr>       | The name of the migration. This is a positional parameter and is required.                                              |
-| <nobr>-OutputDir \<String></nobr> | The directory (and sub-namespace) to use. Paths are relative to the target project directory. Defaults to "Migrations". |
+| -Name \<String>                   | The name of the migration. This is a positional parameter and is required.                                              |


Should probably remove the backslash in front of String if you're wrapping in backticks

roji · 2019-10-15T07:31:54Z

@smitpatel @bricelam can someone approve this or post if you have additional issues?

FYI, was looking at adding a small GitHub Action over on the dotnet repo to just check stuff as it came in

That would be really nice!

nschonni · 2019-10-15T14:29:59Z

FYI, this is the config for the Action setup dotnet/docs#15184

ajcvickers

This looks generally fine to me, but there were two discussions that I pinged @bricelam and @roji on since they don't seem to be resolved.

We can discuss in the design meeting.

Added .markdownlint.json to help keep things clean

nschonni · 2019-10-24T00:03:34Z

FYI, the latest version of markdownlint-cli has added a --fix parameter that can automatically correct some of the rules

roji · 2019-10-24T09:21:39Z

FYI, the latest version of markdownlint-cli has added a --fix parameter that can automatically correct some of the rules

Thanks, we'll start slowly by just merging this PR :)

Out of curiousity, are you using --fix in some automated way in a CI pipeline somewhere? Does that mean that it's actually modifying the commit somehow?

nschonni · 2019-10-24T12:48:38Z

Out of curiousity, are you using --fix in some automated way in a CI pipeline somewhere?

Nope, not on CI, but I've seen that approach in some places with ESLint --fix being used as a pre-commit hook https://github.com/w3c/aria-practices/blob/514af610e735956ada38ac1528dfc2b80ffc6a84/package.json#L44-L58
That approach doesn't do anything for people using GitHub to edit directly, but can be helpful for editors that are developting locally

roji force-pushed the MarkdownLint branch from aec3730 to 1dc3da7 Compare October 2, 2019 14:54

bricelam suggested changes Oct 2, 2019

View reviewed changes

smitpatel reviewed Oct 2, 2019

View reviewed changes

.markdownlint.json Show resolved Hide resolved

smitpatel previously requested changes Oct 2, 2019

View reviewed changes

roji force-pushed the MarkdownLint branch from 1dc3da7 to 800311f Compare October 4, 2019 16:18

roji force-pushed the MarkdownLint branch from 800311f to 53576f7 Compare October 4, 2019 22:43

mairaw reviewed Oct 15, 2019

View reviewed changes

nschonni reviewed Oct 15, 2019

View reviewed changes

roji force-pushed the MarkdownLint branch from 53576f7 to 80026c0 Compare October 15, 2019 07:25

roji mentioned this pull request Oct 15, 2019

Integrate markdownlint into our CI pipeline #1852

Closed

ajcvickers requested changes Oct 22, 2019

View reviewed changes

roji force-pushed the MarkdownLint branch from 80026c0 to 84fbcb1 Compare October 23, 2019 11:06

Run docs through markdown linter

eab3101

Added .markdownlint.json to help keep things clean

roji force-pushed the MarkdownLint branch from 84fbcb1 to eab3101 Compare October 23, 2019 11:14

ajcvickers approved these changes Oct 25, 2019

View reviewed changes

roji merged commit c4d9784 into master Oct 25, 2019

smitpatel deleted the MarkdownLint branch October 25, 2019 16:47

ajcvickers mentioned this pull request Oct 31, 2019

Entity Framework Weekly Status Updates (2019) dotnet/efcore#15403

Closed


		Install the [command-line tools](xref:core/miscellaneous/cli/index):

	\| -Name \<String> \| The name of the migration. This is a positional parameter and is required. \|
	\| `-Name <String>` \| The name of the migration. This is a positional parameter and is required. \|

Run docs through markdown linter #1801

Run docs through markdown linter #1801

Conversation

roji commented Oct 2, 2019

bricelam 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

bricelam Oct 2, 2019 • edited Loading

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

Choose a reason for hiding this comment

smitpatel left a comment

Choose a reason for hiding this comment

bricelam commented Oct 2, 2019

bricelam commented Oct 2, 2019

smitpatel commented Oct 2, 2019

roji commented Oct 2, 2019

bricelam commented Oct 2, 2019

roji commented Oct 2, 2019

roji commented Oct 4, 2019

bricelam commented Oct 4, 2019

smitpatel commented Oct 4, 2019

roji commented Oct 4, 2019

mairaw Oct 15, 2019 • edited Loading

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

mairaw commented Oct 15, 2019

nschonni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji commented Oct 15, 2019

nschonni commented Oct 15, 2019

ajcvickers left a comment

Choose a reason for hiding this comment

nschonni commented Oct 24, 2019

roji commented Oct 24, 2019

nschonni commented Oct 24, 2019

bricelam Oct 2, 2019 •

edited

Loading

mairaw Oct 15, 2019 •

edited

Loading