Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doc: Fix list and nested list rendering issues in spec webpages + other Markdown format cleanup #723

Merged
merged 7 commits into from
Apr 30, 2024

Conversation

bact
Copy link
Contributor

@bact bact commented Apr 21, 2024

Background

Apart from minor incompatibility in Markdown-flavours, GitHub Markdown rendering engine tends to be more robust/error forgiving (can render things that are not a “well-formed” Markdown) but mkdocs (a tool that we use to generate HTML for the spec website) is not so. So some spec description that looks good on GitHub may looks not as good on our spec website.

Compare

You can see that the list in NOASSERTION paragraph is rendered badly on the spec website.

This is because the .md file does not have a blank line between the list and the paragraph before the list. GitHub can tolerate this, but mkdocs cannot.

This PR is trying to avoid the issue (and any issue in the future) by formatting our Markdown files to be well-formed as much as possible.

Changes

Cleanup Markdown formatting to fix document rendering issues on https://spdx.github.io/spdx-spec/v3.0/ , especially on lists and nested lists.

This will fix #720

@bact bact changed the title Fix list and nested list issues in Licensing.md Doc: Fix list and nested list issues in Licensing.md Apr 22, 2024
@bact bact changed the title Doc: Fix list and nested list issues in Licensing.md Doc: Fix list and nested list rendering issues in spec webpages + other Markdown format cleanup Apr 22, 2024
@kestewart
Copy link
Contributor

Thank you very much for figuring this out, and helping us to standarize this. Awesome work.

@kestewart kestewart requested a review from licquia April 23, 2024 23:08
@kestewart kestewart added this to the 3.0.1 milestone Apr 23, 2024
Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all the changes and it looks good

My only concern is if any of the formatting changes trip up the spec parser, but since it passed the CI - it looks like this should work.

@goneall
Copy link
Member

goneall commented Apr 23, 2024

@kestewart @zvr We should probably merge this in soon since it touches soo many files - we don't want to end up with a lot of merge conflicts.

If you could review, then we can merge it in.

Copy link
Contributor

@kestewart kestewart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the changes look good and make sense to me.

Detailed review spotted: model/Core/Vocabularies/_vocab.md still has a TODO in it, that should be sorted out in another request, but that should not block this one from being merged. Agree, sooner rather than later is better here.

We should probably come up with some rules/checks on commits, so this sort of pass isn't required in the future.

@bact
Copy link
Contributor Author

bact commented Apr 24, 2024

We should probably come up with some rules/checks on commits, so this sort of pass isn't required in the future.

There's a GitHub action for a that: https://github.com/marketplace/actions/markdownlint-cli2-action

@bact
Copy link
Contributor Author

bact commented Apr 29, 2024

Is there anything I should change more from the reviews?

The number of following PRs are stacking up and there's a risk of conflicts soon.

@zvr
Copy link
Member

zvr commented Apr 29, 2024

Most of them are cosmetic, but some of them are not.

For example, do not change in the model

Convert headings like Syntax to ### Syntax (MD036)

We don't have headings, we don't have Markdown. We have a specialized input language that looks like Markdown and is processed by spec-parser. Please do not change any structure in the files.

In general, since we do not write Markdown, I see little point in doing checks for well-written Markdown. But of course I won't object to any PR that simply changes the content (but not the structure).

Copy link
Member

@zvr zvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not mark all instances of Constraints and Syntax but these should not be made into new sections

model/Extension/Classes/Extension.md Show resolved Hide resolved
model/Licensing/Licensing.md Outdated Show resolved Hide resolved
model/Licensing/Licensing.md Outdated Show resolved Hide resolved
model/Security/Classes/CvssV2VulnAssessmentRelationship.md Outdated Show resolved Hide resolved
model/Security/Classes/CvssV2VulnAssessmentRelationship.md Outdated Show resolved Hide resolved
model/Security/Classes/CvssV3VulnAssessmentRelationship.md Outdated Show resolved Hide resolved
model/Security/Classes/CvssV3VulnAssessmentRelationship.md Outdated Show resolved Hide resolved
@zvr zvr mentioned this pull request Apr 29, 2024
@bact
Copy link
Contributor Author

bact commented Apr 29, 2024

We don't have headings, we don't have Markdown. We have a specialized input language that looks like Markdown and is processed by spec-parser. Please do not change any structure in the files.

In general, since we do not write Markdown, I see little point in doing checks for well-written Markdown. But of course I won't object to any PR that simply changes the content (but not the structure).

Thank you.

I think that is generally true. It's not a mix of Markdown with something else, not exactly Markdown.

But the list rendering issues do come from the Markdown incompatibility issue, since spec-parser transforms some part of the files in spdx-3-model and copies as-is the other parts.

And those other parts (that went untransformed by spec-parser) can cause incompatibility issue, when they later being consumed by MkDocs.

It's better to go strict as long as it's not a burden of the human editors. A lot of these once merged and having some linter in place will no longer be touched again.

It can be cosmetics, but also machine compatibility and human (editors+readers) readability.

I will fix those suggested.

Btw, thanks for a clear and detailed explanation. It's good to have this discussion since this can be more or less a partial documentation of what spec-parser will consume and will emit. i.e. which parts that we shouldn't touch and leave it as-is in the *.md because it has a specific semantic to the spec-parser. We can copy part of this and put it somewhere in the spdx-spec and spdx-3-model repos so the future editors can be transparently aware of these.

@bact
Copy link
Contributor Author

bact commented Apr 29, 2024

I did not mark all instances of Constraints and Syntax but these should not be made into new sections

Will check all the instances related to that.
Thanks for the heads up.

Signed-off-by: Arthit Suriyawongkul <arthit@gmail.com>
Signed-off-by: Arthit Suriyawongkul <arthit@gmail.com>
@bact
Copy link
Contributor Author

bact commented Apr 30, 2024

@zvr I have revert subsections back as you have suggested. Thanks again for pointing out.

Signed-off-by: Arthit Suriyawongkul <arthit@gmail.com>
@zvr
Copy link
Member

zvr commented Apr 30, 2024

Thanks, @bact .

I'll merge this, since it touches a large number of files and we want to work from this state on.

If any issues arise, they can be fixed in new PRs.

@zvr zvr merged commit 802137f into spdx:main Apr 30, 2024
1 check passed
@bact bact deleted the fix-licensing-list-issues branch April 30, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants