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

Introduce autoformatter #212

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

Krzmbrzl
Copy link
Member

@Krzmbrzl Krzmbrzl commented Aug 4, 2024

This uses Prettier simply because it was the first formatter I found that works with Markdown

Copy link
Member

@Kissaki Kissaki left a comment

Choose a reason for hiding this comment

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

I see it's a nodeJS util, and the CI checks, does not fixup.

Are contributors expected to install this util?

I don't have NodeJS installed, nor do I want to.

I'll be taking a closer look in the coming days.

@@ -0,0 +1,4 @@
{
"printWidth": 120,
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I dislike max line widths quite a bit, especially narrow ones. It see it's at least 120, which could be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

A maximum of 120 is consistent with how we format our C++ code.
I'd also be fine with let's say 150 but I really think line breaking increases readability as you're not dependent on your editor to do auto line wrapping.

@Krzmbrzl
Copy link
Member Author

Krzmbrzl commented Aug 5, 2024

Are contributors expected to install this util?

Not necessarily - they can also manually adjust their formatting.

I see it's a nodeJS util, and the CI checks, does not fixup.

I believe the CI can be configured to do fixups but I usually prefer not splitting change and formatting commits (e.g. have only a single commit that is already formatted).
Though I guess for the website having a super clean commit history is not too important so maybe auto-fixing (on request) is fine.

I don't have NodeJS installed, nor do I want to.

If you have a suggestion for a different Markdown formatter, I'd be open to change to that. It's just that Prettier was the only I could find (though I didn't do an extensive search)

@Kissaki
Copy link
Member

Kissaki commented Aug 28, 2024

Sorry for the delay.

Do you have a sample report/error result? A CI run of it?

I wouldn't add the line length limit after all. There's not necessarily an appropriate formatter available in editing contexts. The markdown files can be considered source code rather than be read as such. Even then on rendering like on GitHub they're not too wide. I prefer manually laying it out, often on sentence borders.

I'm used to * lists, not - lists. It's rendered as bullets after all as well.

Dunno why " is preferred over ' for a head string. Seems to me like ' would be the more deliberately restrictive quoting.

Or why inline-html is line-broken. I prefer line to be a line.

Either way, if you want to merge as is, feel free to. If I end up editing and if it'd actually end up cumbersome I can always raise an issue. :)

It does make me wonder now though, what's the gain in this strict formatting. I'm just a bit worried it's more annoyance than useful.

@Kissaki
Copy link
Member

Kissaki commented Aug 28, 2024

For context, in VS Code I use MarkdownLint https://marketplace.visualstudio.com/items?itemName=DavidAnson.vscode-markdownlint

and I guess it has different (default) rules

Apparently, there's also a GH action of it https://github.com/marketplace/actions/markdownlint-cli2-action

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.

2 participants