guacsec / guac-docs

Docs to ingest in GUAC
https://guac.sh/
Creative Commons Attribution 4.0 International
8 stars 30 forks source link

Change prose-wrap to "preserve" #137

Closed funnelfiasco closed 3 months ago

funnelfiasco commented 3 months ago

Currently, our Prettier config in the CI pipeline uses --prose-wrap always. This forces line wraps when the line exceeds a certain length. For code, that's fine, but for text it leads to awkward commits (like in #129).

Markdown makes more sense in a "sentence-per-line" style, or even Semantic Line Breaks (even though my brain has never adapted to that). I propose we change the pipeline to use --prose-wrap preserve, which leave prose as-is. This will make for clearer diffs while still allowing for other formatting checks.

jeffmendoza commented 3 months ago

While "sentence-per-line" or "Semantic Line Breaks" sounds great in theory. In practice if you remove the forced wrapping CI check, you will end up with markdown files with a full paragraphs on a single line 1000's of characters long. This drives me crazy.

In Emacs I use M-q to re-wrap and keep things nice while I author markdown. I assume any good editor will have similar capabilities. That said, whatever we have in the CI check (npx prettier) will be canonical.

To avoid the need to manually run this, we could add it as a git pre-commit hook as well.

funnelfiasco commented 3 months ago

I'm with you on "full paragraphs on a single line is a scourge", but we can prevent that by rejecting PRs that do this. But there's no way to avoid introducing a bunch of noise into pull requests if a line is edited when using --prose-wrap always. It makes reviewing changes more difficult for no benefit that we couldn't get another way.

jeffmendoza commented 3 months ago

but we can prevent that by rejecting PRs that do this

IME that just doesn't happen.

I prefer the "noise" over long lines. It looks like #129 is the result of main getting out of sync with the CI check requirements, I'm not sure how that happened. In normal cases, just the one paragraph that was edited will be re-wrapped. GitHub's "rich diff" is nice here if the re-wrapping is distracting.

This is my opinion, happy to have others chime in. Keep in mind we should keep the markdown formatting consistent across all the GUAC repos, and we have the same in GUAC here: https://github.com/guacsec/guac/blob/main/Makefile#L113-L116

funnelfiasco commented 3 months ago

Keep in mind we should keep the markdown formatting consistent across all the GUAC repos

I don't think I agree with that. Consistency in style is good as a general rule, but the Markdown in this repo is not for the same purpose as the Markdown in guacsec/guac, for example. But...

This is my opinion, happy to have others chime in.

... while I remain convinced that my opinion is superior 😉 , it's not important enough to push on. I can live with keeping it as-is unless someone else joins in with a strong opinion. If no one has by the time I sit down for work on Monday morning, I'll withdraw my suggestion.