Linting #24
Linting #24
Conversation
|
There was a problem hiding this comment.
Thank you for this pull request, @tkphd! I really appreciate the time you took to put this together.
Unfortunately, I'm going to have to close this because it mixes workflow files and there some adjustments I would like to see (comments inline).
harmonizes line wrapping to the "semantic-and-length" style demonstrated in [CONTRIBUTING.md]
(https://raw.githubusercontent.com/carpentries/workbench-template-md/main/CONTRIBUTING.md),
The CONTRIBUTING document should not be used as the "semantic-and-length" style at the moment. It has some problems that really need to be addressed (but @carpentries/core-team-curriculum has been running a skeleton crew since November and haven't had much time to adjust that).
and appends missing new-lines where needed for POSIX compliance.
Can you explain this a bit further?
Where To contribute
I also wanted to direct you to the right place to adjust these files, because the template files are all sourced from the {sandpaper} R package.
Content files are generated from these templates: https://github.com/carpentries/sandpaper/tree/main/inst/templates
Workflow files are provisioned from these files: https://github.com/carpentries/sandpaper/tree/main/inst/workflows
I don't have the best workflow for updating the template repositories (workbench-template-md and workbench-template-rmd), as that involves me manually making the changes in the repositories.
|
|
||
| # Wrap Markdown at 75 characters | ||
| [*.md] | ||
| max_line_length = 75 |
There was a problem hiding this comment.
I think it might be good to use the .editorconfig from https://github.com/carpentries/styles, as that is the standard our lessons previously conformed to and also does not trim trailing WS in markdown.
Is there a way to exclude the files inside of .github/workflow/?
| @@ -1,3 +1,4 @@ | |||
| --- | |||
There was a problem hiding this comment.
These three lines are in all of the YAML files and I'm not sure how GitHub will react
There was a problem hiding this comment.
GitHub will react just fine, since it conforms to the YAML standard.
|
|
||
| # Which carpentry is this (swc, dc, lc, or cp)? | ||
| # Which carpentry is this? Choose one of: |
There was a problem hiding this comment.
Oh this is much clearer, thank you!
| body: > | ||
| :robot: This is an automated build | ||
|
|
||
| This will update ${{ steps.update.outputs.n }} packages in your lesson with the following versions: | ||
| This will update ${{ steps.update.outputs.n }} packages | ||
| in your lesson with the following versions: | ||
|
|
||
|
|
||
| ``` | ||
|
|
||
| ${{ steps.update.outputs.report }} | ||
|
|
||
| ``` |
There was a problem hiding this comment.
I worry about this change because ${{ steps.update.outputs.report }} is a mutli-line variable (see example in carpentries/sandpaper-docs#132) and it would collapse into a single line if I left it alone or, if I the function that produces the report, then outdated workflows would have a doubling of the number of lines.
| #------------------------------------------------------------ | ||
| # Values for this lesson. | ||
| #------------------------------------------------------------ | ||
| --- |
There was a problem hiding this comment.
Is this an artifact of yaml lint or of markdown lint?
There was a problem hiding this comment.
YAML lint: the YAML standard requires that each YAML document begins with the --- (comments or magics above are OK).
There was a problem hiding this comment.
from what I understand, it's not a requirement per-se, but it could optionally be added as a requirement for a given implementation and is left to opinion.
Neither GitHub nor any of the handlers we use enable the document-start requirement, so it would be better to configure the linter to use document-start: disable (as we do in glosario)
There was a problem hiding this comment.
While it is not regularly enforced, but it is still part of the YAML standard. Since this repository is a template for others to use, I am in favor of stricter adherence to standards; downstream repositories can relax if they so choose.
Also, and perhaps more convincingly, the "---" makes a tidy delineation between the top-level comment explaining what a file is for, and the implementation and section-specific comments in the data itself. For example, config.yaml currently dives right in to "Values for this lesson" which is unclear. We could (should) lead in, along the lines of
# Configuration and template parameters for your lesson.
# Values modified here will propagate through the website
# when you build it, setting the appropriate
# logo/theme, contact info, and episode structure.
---
# -----------------------
# Values for this lesson.
# -----------------------
...There was a problem hiding this comment.
Can you open an issue in {sandpaper} for this? I will revisit after the transition in May.
There was a problem hiding this comment.
Done --> carpentries/sandpaper#439
|
Thanks for the feedback @zkamvar. I can split the edits between separate workflow and regular file PRs, address your comments in the process, and make PRs against the upstream repo(s) where appropriate. |
This PR corrects a few typographical errors, addresses issues raised by MarkdownLint and YamlLint,
harmonizes line wrapping to the "semantic-and-length" style demonstrated in CONTRIBUTING.md,
and appends missing new-lines where needed for POSIX compliance.
To help remove some tedium from maintaining this style, a basic .editorconfig is provided.