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

docs: Format markdown documents #66

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Conversation

gabyx
Copy link
Contributor

@gabyx gabyx commented Jun 12, 2024

Description

Format all markdown docs with prettier.

Reason for the Change

  • All markdown documents should be formatted. I choose normally prettier because there is no better easier tool out there which works nicely. It adheres to CommonMark etc. Ok there is pandoc and other formatters. We can discuss that in BPA for language guidlines on docs maybe.

@cmdoret cmdoret self-requested a review June 20, 2024 15:33
Copy link
Member

@cmdoret cmdoret left a comment

Choose a reason for hiding this comment

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

A markdown formatter woudl indeed be nice to have, but how about using mdformat instead?

I'm suggesting it for the following reasons:

  • it has a pre-commit config, so we could just add it to .pre-commit-config.yaml
  • it is in python, so we can add it to our poetry docs dependency group
  • It is also CommonMark-compliant

They have a readme section on why using mdformat instead of prettier. What do you think?

@gabyx
Copy link
Contributor Author

gabyx commented Jun 21, 2024

@cmdoret : haha, I went already on this journey, I would not recommend mdformat, I tried it in the past, the docs about prettier having troubles is outdated. I used prettier always in my toolchains and never had any trouble. Prettier is at version 3.3 etc and pretty heavily maintained. Although I look for some dedicated/fast tool to format commonmark (pandoc etc. is another beast but to wide-scoped).
So I can just say its stable and works and should support commonmark. Also it is more actively backed by sponsors etc. So no I am 100% not sold on using it.

About the .precommit jeah if you use a python environment, it might give you something stable. But I would not rely on this. Markdown formatting has nothing to do with python itself. So just putting it in the toolchain of python is an easy selling point but jeah. Formatting should be later on be a CI task, stable, pinned and containerized etc.

My suggestion is Githooks, run the formatting in containerized (docker, podman, nix compatible) shared hooks, everywhere , use it in CI as well with some setup..., have it stable.
I would not focus for now on the CI formatting
I could prepare a PR which tries to accomplish to let you give a feel what the benefits are.
If you want to stick to .precommit, for now, all good, I am just a bit worried how it runs on CI, how stable that is etc? Or better I dont understand the .precommit action :), how does it work? it will use a python env and run all hooks with that, if yes this is sluggish and not nice, I d rather vote for something more stable.

@cmdoret
Copy link
Member

cmdoret commented Jun 21, 2024

I see, 👍 for prettier then (as long as it's containerized).

pre-commit is not meant to run on CI: it is only used to setup git pre-commit hooks on the developer's machine. All pre-commit does is automating installation of the hook scripts in .git/.

All good for GitHooks, I have no preference, as long as it automates actions on git commits.

@gabyx
Copy link
Contributor Author

gabyx commented Jun 21, 2024

@cmdoret : Ahh ok, so you dont run it on CI, because you actually can do that =) (its probably a subscription model, payed lol). But dont do that lol.
Jeah, I know how .pre-commit works =), Githooks is written by me, I know all the shittyness of Git hooks and their niceities etc.

Software Best Practice Goal for every project I wanna drive further is: CI runs exaclty the same as you run locally, no excuses ;) (beeing pragmatic here, ok minimal differences are allowed, but not with version pinning etc., I have enough painful experiences with these topics lol)
CI is ground truth. Tools like Git hooks are nice! but if used they MUST align with CI 100%, so jeah Githooks can do that.
I would like however to include a Nix feature in Githooks too, such that we do not need nested container feature.

@cmdoret cmdoret self-requested a review June 24, 2024 11:34
Copy link
Member

@cmdoret cmdoret left a comment

Choose a reason for hiding this comment

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

✔️ for the prettier setup, we can figure out pre-commit / ci separately

@cmdoret cmdoret merged commit 4f11c90 into main Jun 26, 2024
7 checks passed
@almutlue
Copy link
Contributor

almutlue commented Jun 26, 2024

Thanks for this @cmdoret, @gabyx! I'm sorry for being late to the discussion. It looks like we are running into rendering issues for our docs when using prettier (see here). I thought that prettier and sphinx would work with each other, but maybe not all the extensions we use (e.g. myst parser)? I did not directly find a good explanation when googling. What do you think we should do? Rollback and scope prettier to markdowns outside docs?

@almutlue
Copy link
Contributor

It is also a bit disappointing that our checks did not catch this - it is true that the docs are technically building, but still a warning would have been nice 😞

cmdoret added a commit that referenced this pull request Jun 26, 2024
cmdoret added a commit that referenced this pull request Jun 26, 2024
This reverts commit 4f11c90 which broke docs rendering
@gabyx
Copy link
Contributor Author

gabyx commented Jun 27, 2024

Oh, yes thats wos dumb. Good that you checked.

May be we should ignore the prettier in the sphynx doc etc.

@gabyx
Copy link
Contributor Author

gabyx commented Jun 28, 2024

It is also a bit disappointing that our checks did not catch this - it is true that the docs are technically building, but still a warning would have been nice 😞

I think it cannot, markdown is pretty agnostic thats also good, (not like latex where you get shitty errors) so it will render but corrupted... it is how it is =). Or what checks to you relate to?

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