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

Fixes codeblock indentation in order to make them actually work #184

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

arielf212
Copy link
Contributor

Currently the documentation website shows broken code blocks, this PR provides a fix to that :)

Copy link

netlify bot commented Nov 26, 2024

Deploy Preview for aya-rs ready!

Name Link
🔨 Latest commit 938a943
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs/deploys/67470ea5c70d620008e4e010
😎 Deploy Preview https://deploy-preview-184--aya-rs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@vadorovsky vadorovsky left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the fix! 🙂

Unfortunately, markdownlint complains about that:

Error: docs/book/programs/cgroup-skb.md:43 MD046/code-block-style Code block style [Expected: indented; Actual: fenced] https://github.com/DavidAnson/markdownlint/blob/v0.36.1/doc/md046.md

But honestly, I think this error is 🐂 💩, since the "indented" code blocks simply don't work.

So I think we could just try to ignore that error. According to:

https://github.com/DavidAnson/markdownlint-cli2-action?tab=readme-ov-file#config-optional
https://github.com/DavidAnson/markdownlint-cli2

We could try changing the markdownlint job definition in .github/workflows/ci.yml to:

      - uses: DavidAnson/markdownlint-cli2-action@v18
        with:
          config: .markdownlint.json
          globs: '**/*.md'

And then add the .markdownlint.json file with the following content:

{
  "code-block-style": {
    "style": "fenced"
  }
}

I think that should fix the CI failure.

@arielf212
Copy link
Contributor Author

Done!
Thank you for your guidance! :)

Copy link
Member

@vadorovsky vadorovsky left a comment

Choose a reason for hiding this comment

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

Thanks!

CI is now rightfully pointing out some places where code blocks are indented. Can you remove the indentation there as well?

@arielf212
Copy link
Contributor Author

Yeah, just hopped in to see what the CI is yapping about now.
Seems like I can't fix most of those problems, since that would break the intended look of the page.
In general, I am not sure this utility is very useful for mkdocs, since mkdocs uses a lot of non-standard markdown extensions :)

@arielf212
Copy link
Contributor Author

I found myself copy-pasting a lot of enable/disable pragmas into the codebase, which I found really ugly.
As such, I just disabled two very noisy rules and fixed the remaining one.
As I described in the commit message, the codeblock rule fundamentally clashes with how mkdocs works,
while the second rule (dollar sign) clashes with the already-set convention for this repo.

@arielf212 arielf212 requested a review from vadorovsky November 26, 2024 16:10
@arielf212 arielf212 requested a review from tamird November 26, 2024 16:43
@vadorovsky
Copy link
Member

vadorovsky commented Nov 26, 2024

In general, I am not sure this utility is very useful for mkdocs, since mkdocs uses a lot of non-standard markdown extensions :)

To be honest, the only reason I decided to introduce it is that we used to have a lot of places with over 200 charaters per line, weird whitespaces and inconsistent heading. And also a lot of PR introducing more of them. 😅 I'm not a big fan of nitpicking such stuff as a person (it gives people room to come up with completely arbitrary rules), so I would rather have a linter making sure that *.md files aren't unreadable. I agree that some markdownlint rules are questionable and not compatible with mkdocs. Unfortunately, I don't think there is a good alternative. So I think we just need to figure out a minimal set of rules which make sense for us to use.

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

LGTM. Mind squashing these down to a single commit with a sensible message?

@@ -0,0 +1,3 @@
{
"code-block-style": false
Copy link
Member

Choose a reason for hiding this comment

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

oh wait, can we make this fenced? or why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having this be fenced messes with places which have to be indented, like inside an admonition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, squashed

Copy link
Member

Choose a reason for hiding this comment

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

There's some advice in DavidAnson/markdownlint#207 about how to support admonitions. Any interest in making that work in this PR?

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Alright, we can tackle the admonition thing in another PR.

@arielf212
Copy link
Contributor Author

@tamird Seems like the "solution" provided in the link you linked isn't actually relevant to our situation since you can't load plugins in the cli.
As such, there is no easy fix for this as far as I see :)

@tamird
Copy link
Member

tamird commented Nov 27, 2024

Could you resolve the conflict?

Currently markdownlint (which runs in our CI/CD) refuses non-indented code blocks,
but material mkdocs requires code blocks to not be indented.

As such, we will ignore this lint rule.

mkdocs clashes really hard with the code block related lint and as
such I disabled it.
@arielf212
Copy link
Contributor Author

I would really love to stop chasing this PR, so it'd be nice if you could merge it and not merge new stuff in-between, especially since this is a very minor change.

@tamird tamird merged commit dd139f7 into aya-rs:main Nov 27, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants