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

Reformat README #8338

Merged
merged 4 commits into from
Mar 7, 2024
Merged

Reformat README #8338

merged 4 commits into from
Mar 7, 2024

Conversation

mnonnenmacher
Copy link
Member

Please see the commit messages for details.

@mnonnenmacher mnonnenmacher force-pushed the reformat-readme branch 2 times, most recently from 601cac2 to a8578fe Compare February 27, 2024 11:39
@mnonnenmacher mnonnenmacher marked this pull request as ready for review February 27, 2024 11:54
@mnonnenmacher mnonnenmacher requested review from a team as code owners February 27, 2024 11:54
Copy link
Member

@sschuberth sschuberth left a comment

Choose a reason for hiding this comment

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

I'm ok to take it into use, but we should ASAP implement checks to enforce this (for already migrated files).

@mnonnenmacher
Copy link
Member Author

I'm ok to take it into use, but we should ASAP implement checks to enforce this (for already migrated files).

Like I mentioned in the core dev meeting, the problem is that there is no rule for that. There are two markdownlint plugins which we might use, but both of them don't seem to be actively maintained and I don't know how well they work:
https://www.npmjs.com/package/markdownlint-rule-max-one-sentence-per-line
https://www.npmjs.com/package/sentences-per-line

I could give it a try anyway.

This allows to use the one sentence per line formatting to avoid that
changing one sentence requires reformatting a whole paragraph.

Signed-off-by: Martin Nonnenmacher <[email protected]>
@sschuberth
Copy link
Member

What's blocking to merge this @mnonnenmacher?

@mnonnenmacher
Copy link
Member Author

What's blocking to merge this @mnonnenmacher?

I wasn't sure if you are stil ok with the merge in case it does not work to use one of the rules mentioned above, and I haven't found the time to test it yet.

@sschuberth
Copy link
Member

sschuberth commented Mar 7, 2024

I wasn't sure if you are stil ok with the merge

I'm still fine. Let's merge, and if the feel comfortable with the style, come up with automated checks and roll it out further. And if we are not fine, this change is small enough to revert (even manually).

@mnonnenmacher mnonnenmacher merged commit 5cbb03c into main Mar 7, 2024
21 checks passed
@mnonnenmacher mnonnenmacher deleted the reformat-readme branch March 7, 2024 14:28
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