correct html closing tag (typo)

[kristy-hu]

Correct a typo on <summary> page.

Description

Motivation

Additional details

Related issues and pull requests

[github-actions]

Preview URLs

• /en-US/docs/Web/HTML/Element/summary

(comment last updated: 2024-12-16 08:32:16)

[chrisdavidmills]

@kristy-hu hello there, and thank you for your contribution to MDN!

Your fix looks correct to me, but the linter is complaining and giving a totally nonsense error message.

I would try fixing up the edit for the <q> element (to <q> ... </q>, or maybe put the tags on separate lines to the content, whatever you think looks best) and then push that. If this still fails (I'm wondering if it was the linter that suggested that weird fix, it sometimes does weird things), then add -nolint on to the opening code fence label, i.e. html-nolint to get the linter to skip linting this code block.

[Josh-Cena]

Your fix looks correct to me, but the linter is complaining and giving a totally nonsense error message.

The formatter is stable and predictable. The problem here is that because this PR fixes a syntax error, suddenly the formatter is able to run on a big chunk of code, but reviewdog cannot comment on unchanged lines. To fix errors like this, you would need to either paste this file into Prettier playground (https://prettier.io/playground) or format it locally.

[chrisdavidmills]

@Josh-Cena yes, it's stable and predictable, but some of its error messages are hard to decipher.

And, the edit to the HTML has still resulted in this weird breaking of tags across lines, which I think needs to be fixed. You'd use that style when you have a lot of attributes, putting each attribute on a separate line, but not in this case, where there are no attributes.

[kristy-hu]

@chrisdavidmills Hi! I see now the <q> tag has been unnecessarily broken into two, which isn't very pretty... Does -nolint work? Or is there any way to fix the <q> now the syntax is correct?

[Josh-Cena]

And, the edit to the HTML has still resulted in this weird breaking of tags across lines, which I think needs to be fixed. You'd use that style when you have a lot of attributes, putting each attribute on a separate line, but not in this case, where there are no attributes.

That's because we treat HTML as whitespace-sensitive. Formatting it as

<q>
  foo
</q>

Is pretty, but actually results in extra whitespace around the text. We have decided that correctness is more important than source code beauty.

[Josh-Cena]

I agree that the error message is extremely hard to decipher though. @OnkarRuikar I've probably reported the same thing before, but Prettier errors are not surfaced as such in the log:

Logs from markdownlint:
yarn run v1.22.22
$ /home/runner/work/content/content/node_modules/.bin/markdownlint-cli2 --fix files/en-us/web/html/element/summary/index.md
markdownlint-cli2 v0.16.0 (markdownlint v0.36.1)
Finding: files/en-us/web/html/element/summary/index.md !node_modules !.git !.github !tests
Linting: 1 file(s)
Summary: 0 error(s)
Done in 0.[29](https://github.com/mdn/content/actions/runs/12348529070/job/34457537052#step:12:30)s.

Logs from front-matter linter:
0 0

Please fix all the linting issues mentioned in above logs and in the review comments.
Error: Process completed with exit code 1.

[chrisdavidmills]

And, the edit to the HTML has still resulted in this weird breaking of tags across lines, which I think needs to be fixed. You'd use that style when you have a lot of attributes, putting each attribute on a separate line, but not in this case, where there are no attributes.

That's because we treat HTML as whitespace-sensitive. Formatting it as

<q>
  foo
</q>

Is pretty, but actually results in extra whitespace around the text. We have decided that correctness is more important than source code beauty.

@Josh-Cena Yes, but

<q>foo</q>

would avoid the ugly broken tags issue, AND fix the whitespace issue. Can we not do that instead? I know it's too late for this one, as you've merged it, but in future ... ?

[Josh-Cena]

The formatter does not allow any subjective preferences for formatting, unfortunately. It ignores any line breaks and reformats everything. I do prefer tags to stay in one line but that's not the style chosen by Prettier and we would have to respect it.

[OnkarRuikar]

About the prettier diff was not suggested for the first commit, the workflow code does work fine. It could be a GitHub glitch. Was this Kristy's first PR submission? For investigation, how often have you seen Reviewdog not commenting on PR, but there are Prettier suggested changes?

I agree that the error message is extremely hard to decipher though. @OnkarRuikar I've probably reported the same thing before, but Prettier errors are not surfaced as such in the log:

Prettier doesn't throw errors in write mode. It just modifies files. Will dumping diff after the error message help?

[kristy-hu]

Was this Kristy's first PR submission?

No, actually it was the second here on MDN XD.

[Josh-Cena]

Prettier doesn't throw errors in write mode. It just modifies files. Will dumping diff after the error message help?

Why don't we run Prettier with --check instead??

It could be a GitHub glitch. Was this Kristy's first PR submission? For investigation, how often have you seen Reviewdog not commenting on PR, but there are Prettier suggested changes?

The would-be diff is physically unable to be commented on, because it's too far from the changed lines; I doubt reviewdog will be able to bypass GH limits.

[chrisdavidmills]

The formatter does not allow any subjective preferences for formatting, unfortunately. It ignores any line breaks and reformats everything. I do prefer tags to stay in one line but that's not the style chosen by Prettier and we would have to respect it.

And most of the time the job is done fine. But in situations like this, where you end up with screwy markup formatting, I think it is OK to override it with -nolint.

[OnkarRuikar]

Why don't we run Prettier with --check instead??

Reviewdog doesn't come into the picture when contributors do commits from the local dev environment. The workflow is mainly for cases where a commit is made using GitHub. So, it is preferable to show the expected changes directly than to tell a file needs to be formatted.

We could run prettier --check at the end of a workflow run if it fails and ask contributors to get the file content formatted using the prettier playground.

[Josh-Cena]

Yes that sounds good