Skip to content

Style guide for collaborative_workflow #71

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

Closed
kellijohnson-NOAA opened this issue Mar 17, 2022 · 8 comments
Closed

Style guide for collaborative_workflow #71

kellijohnson-NOAA opened this issue Mar 17, 2022 · 8 comments
Labels
kind: developer handbook update describes a new component of or change to developer book
Milestone

Comments

@kellijohnson-NOAA
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
A style guide is needed for the markdown files that make up the collaborative_workflow book.
Mainly, it is unclear if we should be wrapping lines or not.
Unwrapped lines make the text difficult to read on GitHub,
though this is only a small concern to me.
Guidance would be needed on where and when to wrap lines if we wanted to go that route.
Largely, I just want to know how to best make contributions in a consistent, logical, and appreciated way.

Describe the solution you'd like
An automatic linter could be added as a GitHub Action.
The gh would formats the markdown files before they were merged into main.

Describe alternatives you've considered
I thought about adding information to the following file

The FIMS project uses style guides to ensure our code is consistent, easy to use (e.g. read, share, and verify), and ultimately easier to write. We use the [Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html) and [Google's R Style Guide](https://google.github.io/styleguide/Rguide.html). Google's R Style Guide is based off of the [tidyverse style guide](https://style.tidyverse.org/), with a few minor modifications to improve readability and portability.

regarding a style guide for this document. But,
that requires people to read and implement, where a linter would do things automatically.

I am not sure if there is an action available for .rmd files but there certainly is for .md files.

Additional context
NA

@kellijohnson-NOAA kellijohnson-NOAA added topic: documentation kind: developer handbook update describes a new component of or change to developer book labels Mar 17, 2022
@kristanblackhart-NOAA
Copy link
Contributor

@kellijohnson-NOAA good suggestion.

For .md files, this looks like a good option https://github.com/marketplace/actions/markdown-lint
It provides readability and consistency without too many rules (md linters include API style which could be overly prescriptive for our purposes).

I cannot find an action available for .rmd files.

@k-doering-NOAA
Copy link
Member

{styler} also works on .rmd files, so we could use this workflow: https://github.com/nmfs-fish-tools/ghactions4r/blob/main/.github/workflows/style-r-code.yml

I'm not sure what it does with line wrapping, though.

Personally, I don't like line wrapping, I can understand why others do. Maybe if I knew how to automatically line wrap and save the results using an IDE I wouldn't mind it!

@k-doering-NOAA
Copy link
Member

I checked out styler and couldn't find a way to make it break .Rmd lines at 80 characters; I think @iantaylor-NOAA mentioned his IDE can break lines automatically for him, so I wonder if we could start by having someone run the code through someone's local tools (if we do want to use line breaks), then we can figure out how to set up something automatic (which I imagine must exist, but we probably could write some code to do this...)

@iantaylor-NOAA
Copy link
Contributor

@kristanblackhart-NOAA, it looks like that github action applies this set of rules: https://github.com/DavidAnson/markdownlint#rules--aliases
(which among many other things, include breaking lines at 80 characters by default:
https://github.com/DavidAnson/markdownlint/blob/main/doc/Rules.md#md013---line-length)

markdownlint is also implemented as a vscode extension with the same name. I tried it out by installing the extension, then saving 07-contributor-guide.Rmd as an *.md file which brought up 42 items in the PROBLEMS pane.

At first glance, the issues identified mostly seem reasonable, but I think would be annoying to have them all implemented automatically via github action. For instance, it raised issues with the R code chunks (really git command-line commands in this file, but would have been similar if it were R, I think). I now see the suggestion from @k-doering-NOAA about the styler package, which might be a better choice.

Regarding wrapping, the main reason why I like line breaks is to help with comparing changes in GitHub. For instance, this screenshot from PR #41 shows the a comparison to a big paragraph that was on a single line with no indication of where some minor edits occurred (from https://github.com/NOAA-FIMS/collaborative_workflow/pull/41/files):
image

@kristanblackhart-NOAA
Copy link
Contributor

The styler package still does not break long lines, which seems to be the main goal of implementing a linter for md/rmd files. Fine with not using the markdown-lint action if it's going to be a headache for everyone. Other ideas?

iantaylor-NOAA added a commit that referenced this issue Mar 21, 2022
- uses vscode Rewrap extension
- required a little tinkering, but not much
@iantaylor-NOAA
Copy link
Contributor

Linebreaks added for consideration in PR #75 using the Rewrap vscode extension.

For future reference, here is info on doing this to the Pacific Hake repository using in 2018 using Emacs: pacific-hake/hake-assessment#305 (comment) where two things to watch out for are noted: harder to search for a specific phrase if that gets split across lines and trailing comments could get messed up. Neither seems like a big deal in this case.

@k-doering-NOAA
Copy link
Member

Adding this link for later reference, indeed styler does not offer line breaks and seems like it won't anytime soon: r-lib/styler#247 (comment)

ChristineStawitz-NOAA pushed a commit that referenced this issue Mar 26, 2022
* feat: break lines in .Rmd files at 72 chars #71
- uses vscode Rewrap extension
- required a little tinkering, but not much

* fix: line break in developer heading

Co-authored-by: Kathryn Doering <[email protected]>
@kellijohnson-NOAA kellijohnson-NOAA added this to the M2 milestone Jun 14, 2024
@kellijohnson-NOAA
Copy link
Collaborator Author

We should edit paragraphs to be a single line as we update the repository. Over time the consistency will improve and contributors will use the style that is in place.

There is no automated way to hard wrap at 80 characters with a .Rmd file right now but you can soft wrap in your editor (VS Code is Alt Z) for those that like having the lines wrapped. This is even possible when using VS Code on GitHub. I do not want to edit every paragraph in a single commit right now because we are proposing to move some of the text soon so I see that as a wasted effort.

Closing for now but we can always revisit later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: developer handbook update describes a new component of or change to developer book
Projects
None yet
Development

No branches or pull requests

4 participants