Skip to content
This repository has been archived by the owner on Sep 7, 2021. It is now read-only.

Lint markdown files; adjust headings #44

Merged
merged 4 commits into from
Jun 4, 2019
Merged

Lint markdown files; adjust headings #44

merged 4 commits into from
Jun 4, 2019

Conversation

Elchi3
Copy link
Member

@Elchi3 Elchi3 commented May 31, 2019

Another large PR. I'm very sorry about this :(

I was happy to see we're going from HTML comments to headings to demarcate sections 🎉

However, looking into headings and markdown, I was super unhappy with how there are two heading styles (settext and atx). And of course, because linting in this repo is limited still, we have already used both of them. I dislike the settext style because it is very implicit and it doesn't support all heading levels but just h1 and h2. So, we will always have to also use atx style heading as well.
Now we're also using headings as one of our main feature to demarcate content, so I believe it is time to lint this and be strict about it. So this PR does a bunch of things to help us to meet content expectations.

  • Change comments to atx style headings
  • Enforce atx heading style per linter
  • Add remark-frontmatter, so that the linter understands that we're using frontmatters and that it doesn't confuse it with settext style headers which we disallow.
  • remark-lint doesn't cause the build to fail on warnings normally, but I think we should be strict and have it fail, as our project fails and wins with well formatted and structured content (this is the -f option).
  • remark-lint is quite verbose and always lists passed files, I think we don't need this (this is the -q option)
  • Add in "remark-preset-lint-consistent". This looks like a sensible package that helps us to avoid further inconsistencies. It brings the consistent heading styles (and this is the reason I added it, but it also has other things to help being consistent)

Somewhat unrelated is fixing all the other linting errors as I went through this, sorry about that, but I didn't want to go through the files twice to fix what the linter currently yells at us.

Oh, one question that I had while doing this: What to do whith empty sections? Do we want to have the headline or not? We did have empty html comment sections, so I've preserved this and added empty sections that start with a heading. Let me know if we should rather remove them.

@peterbe
Copy link
Contributor

peterbe commented May 31, 2019

@Elchi3 how does this intersect with #41 ?

@peterbe
Copy link
Contributor

peterbe commented May 31, 2019

Note-to-self; this will conflict with #42 whichever lands first.

@Elchi3
Copy link
Member Author

Elchi3 commented May 31, 2019

@Elchi3 how does this intersect with #41 ?

It is supplementary. It applies the change throughout and adds "formatting linting" specifically for headings. Next step is to actually add "content linting" that guarantees certain headings like "Short description" per the recipe rules, etc.

@peterbe
Copy link
Contributor

peterbe commented May 31, 2019

Next step is to actually add "content linting" that guarantees certain headings

Already filed here: #38

@Elchi3 There, I wrote "I'm happy to take a stab at this one." which you can ignore :)

Nothing's black-and-white but I like this healthy dynamic that is developing. That the MDN content team makes sure stumptown's content is stellar (not just in actual content but also in structured perfection) and the MDN dev team (currently, to be honest, me and Will) then assumes stumptown to be perfect and then runs with that assumption. Seems like a healthy boundary. Mind you, it's not healthy if consumers and producers don't communicate either.

@Elchi3
Copy link
Member Author

Elchi3 commented May 31, 2019

Already filed here: #38
@Elchi3 There, I wrote "I'm happy to take a stab at this one." which you can ignore :)

I didn't see that, but I also didn't start any work on that specifically yet. Feel free to take over.

Mind you, it's not healthy if consumers and producers don't communicate either.

Sorry about that. Last time I was here, this was still very much an experiment, and for me I just hack away on experiments. Ideally with a somewhat creative flow in which I then don't always ask for permission. Again: sorry.

I didn't participate here in months as I have ambitious OKRs that don't really allow me to lose focus on them. I took some time this week as there was a holiday and I wanted to see where things are before I come to Whistler. As said in my other PRs, feel to ignore my PRs or just take them as inspiration.

@peterbe
Copy link
Contributor

peterbe commented May 31, 2019

Sorry about that.

Nothing to be sorry for. I didn't nitpick that the issue was already filed. Just trying to help.

Also, what I meant about communicating between consumers and producers is that if any human tries to fully understand both sides they'd be stretched too thin. But a little stretching is good.

I really wish everyone on the whole team gets a chance, before meeting in Whistler, to poke more at stumptown and the renderer since it would genuinely help all discussions if everyone surfaces rather than a select few in a skunkworks bunker.

@wbamberg
Copy link

Thank you @Elchi3 !

This is a big PR but well-explained and there's nothing I disagree with.

I was super unhappy with how there are two heading styles (settext and atx). And of course, because linting in this repo is limited still, we have already used both of them. I dislike the settext style because it is very implicit and it doesn't support all heading levels but just h1 and h2.

  • Change comments to atx style headings
  • Enforce atx heading style per linter

(settext/atx are new to me terms). I agree with all this. I'm going to blame the settext style ones, and the other problems you have fixed here, on Pandoc, again.

I think once we get into migration mode we'll have to have a stern talk with Pandoc.

@Elchi3 how does this intersect with #41 ?

I guess this PR will break stumptown as it stands, because it needs #41 to handle the change in section demarcation. And #41 will probably also conflict with this one, since it also updates the headings in video's prose.md.

And #41 will also break mdn2 because it changes the JSON representation for prose sections.

And also also, mdn/yari#3 still isn't merged, and fixing mdn2 for this breakage will destroy that patch.

So I feel like the order here ought to be:

  1. merge Add some renderers yari#3
  2. merge Use headings rather than comments to demarcate prose sections #41. This then breaks mdn2, so:
  3. file a new PR to update mdn2 to understand the new JSON structure
  4. once that is merged, merge this PR (having sorted out any conflict from Use headings rather than comments to demarcate prose sections #41)

Does that sound good?

@Elchi3 , I think this is really great work to update all the section demarcators. In my more optimistic moments I think maybe by the end of the sprint we could have not just one page rendering but 50, and this is the kind of change we need to get there :). It's also great to see you working on the linting.

@wbamberg
Copy link

What to do with empty sections?

We should not allow empty sections. We already allow sections to be optional. If there is a good reason sometimes for a page to omit a section, then we should make the section optional. Allowing empty mandatory sections makes a mockery of the whole thing :).

So in concrete terms, say:

  • <html> doesn't have "Overview" - it just goes from short description into attributes.
  • <ins> doesn't have "Usage notes"

Do we think these pages are defective? Should we write sections for them? Or are they fine as they are, in which case we should make these sections optional. This is really a content design question, and I'm not sure of the answer (and we should also consult with @chrisdavidmills , who was mostly responsible for designing the structure of HTML element pages that we are following here - although I see now that this does not include "Usage notes", oops).

@wbamberg
Copy link

Next step is to actually add "content linting" that guarantees certain headings like "Short description" per the recipe rules, etc.

Yes. My comment at #38 (comment) captures my view of this. I need to take some time to try to write down what "correctness" is for everything. I wasn't intending to do that in this sprint but perhaps I should after all.

That the MDN content team makes sure stumptown's content is stellar (not just in actual content but also in structured perfection) and the MDN dev team (currently, to be honest, me and Will) then assumes stumptown to be perfect and then runs with that assumption. Seems like a healthy boundary.

Yes :), except I'm not really in the dev team.

Copy link

@wbamberg wbamberg 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 good to me, but I'd like to merge some other PRs first, as said in #44 (comment).

@Elchi3
Copy link
Member Author

Elchi3 commented Jun 2, 2019

Thanks for your encouraging words, Will! Makes me want to continue with this when I can find the time.

I think once we get into migration mode we'll have to have a stern talk with Pandoc.

Yes, this. I haven't looked into pandoc settings a lot, but it looks like we need to dive deeper into it, so that we produce nice and consistent markdown. I think this will also become more clear once we have chosen a flavor, but I also think working more with the markdowns and getting to know the details like settext and atx (which were new terms to me as well, btw) is going to help us a lot making good decisions and deciding on one or another flavor or style.

We should not allow empty sections.

I think this makes sense and I can remove these in this PR if you like or follow up on it.

Do we think these pages are defective? Should we write sections for them?

This is indeed a content design question, but however we decide I am so impressed that we could now imagine tooling that would help us enforce updated content design questions. So, say we make them optional for now, but then it turns out that people really have a need for these sections. We could then turn on stricter linting that requires the sections and update the content to meet the criteria. So powerful! The recipes/linter will kind of have our content design explicitly stated!

And #41 will probably also conflict with this one, since it also updates the headings in video's prose.md.

I think this conflict won't be too bad, happy to resolve that when you land #41 first.

@wbamberg
Copy link

wbamberg commented Jun 3, 2019

  1. merge Add some renderers yari#3
  2. merge Use headings rather than comments to demarcate prose sections #41. This then breaks mdn2, so:
  3. update mdn2 to understand the new JSON structure

These are all done, and I've fixed the merge conflict with this one (and then fixed an error I inadvertently introduced in the process).

So we are able to merge this. But I think we should remove the empty sections first. We won't get errors from them yet of course. Please let me know if you don't have time to do this, and I will do it so this PR can be merged.

Thanks again Florian.

(It sounds like HTML element content design might be a good session for the All-Hands.)

@Elchi3
Copy link
Member Author

Elchi3 commented Jun 4, 2019

These are all done, and I've fixed the merge conflict with this one (and then fixed an error I inadvertently introduced in the process).

Awesome, thanks Will!

So we are able to merge this. But I think we should remove the empty sections first. We won't get errors from them yet of course. Please let me know if you don't have time to do this, and I will do it so this PR can be merged.

I've just done that. As you say, no validation for this yet, so maybe I missed some, but hopefully not :)

(It sounds like HTML element content design might be a good session for the All-Hands.)

Yes, seems like a good idea. Would also like further brainstorm on (linting) architecture (see my comment on #38 (comment))

@wbamberg wbamberg merged commit 9b70659 into mdn:master Jun 4, 2019
@Elchi3 Elchi3 deleted the lint-md branch June 4, 2019 12:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants