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

Enable automatic formatting of the guide #406

Merged
merged 3 commits into from
Dec 19, 2024
Merged

Enable automatic formatting of the guide #406

merged 3 commits into from
Dec 19, 2024

Conversation

bouweandela
Copy link
Member

@bouweandela bouweandela commented Dec 6, 2024

Changes in this PR

Automatically format files using prettier and pre-commit.

Relevant changes are in d242da8, changes caused by automated formatting in 6230e3f. The broken links are not new in the pull pull request and in part caused by too much traffic coming from our CI.

Closes #119

Checklist

SIGNIFICANT changes / additions, e.g. new chapters

  • I checked whether the contribution fits in The Turing Way before considering contributing to this Guide.
  • I discussed my contribution in an issue and took into account feedback.

ALL contributions

  • I previewed my changes locally using e.g. python3 -m http.server 4000 and confirmed they work correctly.
  • I checked for broken links, e.g. using the link checker GitHub Action workflow, or locally by using docker run --init -it -v `pwd`:/docs lycheeverse/lychee /docs --config=docs/lychee.toml, at least for the files I changed.
  • My name was added to the CITATION.cff file.

@bouweandela bouweandela marked this pull request as ready for review December 6, 2024 10:57
@bouweandela bouweandela force-pushed the automatic-formatting branch 3 times, most recently from fbfb552 to 6230e3f Compare December 6, 2024 11:01
@bouweandela bouweandela requested a review from egpbos December 6, 2024 11:05
Copy link
Member

@egpbos egpbos left a comment

Choose a reason for hiding this comment

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

This is a nice addition! I like it especially for the yml, html and css, no comments there. I cannot work with the Markdown style, though, so I hope we can change or disable that (at least for now).

.prettierrc.yaml Outdated Show resolved Hide resolved
.github/workflows/link-checker.yml Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated
Comment on lines 12 to 14
This guide is primarily written by the Research Software Engineers at the
Netherlands eScience Center. Contributions by anyone (also outside the Center)
are most welcome!
Copy link
Member

Choose a reason for hiding this comment

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

This style choice I have to kinda strongly disagree with :) In Markdown (also in LaTeX), I like to have each sentence on its own line so that git diff views don't get cluttered by meaningless line break shifts (as we see here). Can we disable this? I don't necessarily want to enforce my style choice, but at least let's discuss it separately before enabling here.

Copy link
Member

Choose a reason for hiding this comment

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

I will not review the rest of the markdown files right now. I cannot easily browse changes this way, illustrating my point :P

Copy link
Member Author

@bouweandela bouweandela Dec 6, 2024

Choose a reason for hiding this comment

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

Having long or many sentences on a single line leads to unreadable diffs as well, so that's why I thought that limiting the line length might help. However, it only helps if you also start every new sentence on it's own line, because then the amount of words that shift will be limited to a single sentence which is still kind of awkward but not too bad. Unfortunately the formatter does not seem able to do that. I tried mdformat as well, but the results are similar (see #411).

Shall I just remove the line length limit for now? Or should we write in the contribution guidelines that people have to start every new sentence on a new line and I do it for the existing text in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove the line length limit, indeed, and discuss the line style later. As I said, I don't really want to force this style upon people, I feel it overcomplicates the contribution process (I'm happier with ugly Markdown than with no Markdown :) ). But maybe I'm too cautious. I'd prefer to get a few more opinions, though. I will make a separate issue for this.

Copy link
Member

Choose a reason for hiding this comment

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

#416.

Copy link
Member Author

Choose a reason for hiding this comment

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

Disabled in 5d33c2a

This was referenced Dec 10, 2024
Copy link

netlify bot commented Dec 19, 2024

Deploy Preview for nlesc-guide-testing ready!

Name Link
🔨 Latest commit 1f82414
🔍 Latest deploy log https://app.netlify.com/sites/nlesc-guide-testing/deploys/6763e38f03d53c0008429f3d
😎 Deploy Preview https://deploy-preview-406--nlesc-guide-testing.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.

@bouweandela
Copy link
Member Author

@egpbos This is ready for another round of review.

Copy link
Member

@egpbos egpbos left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @bouweandela! One minor comment for consideration. I did not check all the markdown files in detail (did you?), so let's be alert for possible weirdness.

CONTRIBUTING.md Show resolved Hide resolved
@bouweandela
Copy link
Member Author

did not check all the markdown files in detail (did you?)

No, just looked at a couple of files

@egpbos egpbos merged commit b62eaa5 into main Dec 19, 2024
19 of 21 checks passed
@egpbos egpbos deleted the automatic-formatting branch December 19, 2024 14:43
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.

Coding style for markdown files
2 participants