-
Notifications
You must be signed in to change notification settings - Fork 17
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
Use multiple markdownlint.yaml /versions and /schemas #115
base: main
Are you sure you want to change the base?
Use multiple markdownlint.yaml /versions and /schemas #115
Conversation
MD024: false # duplicate headings | ||
MD032: false # blanks-around-lists Lists should be surrounded by blank lines | ||
MD033: false # inline HTML | ||
MD047: false # single-trailing-newline Files should end with a single newline character |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rules 32 and 47 are disabled because they didn't pass. We can enable them again and format the file if that is what we desire.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please use the same rules for all *.md
files and change the files that don't follow the rules yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I agree that the same rules apply for specification documents and our internal project documentation in markdown files. I think we already rejected a pull request that applied the same standards to both - We need to be strict and careful with the documents that are published, I'm not sure the same applies if people are adding different content types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @ralfhandl , I am not against in what you are proposing but @lornajane is right about the rejected PR. Please check #105
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the commented-out rules are trivial to observe, I'd use the same rules.
The more interesting case will be when we make the rules for new spec documents stricter and will need different rule sets for files in the versions
folder, as we already do for OAS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ralfhandl Ok. But what this means for the other comments that you made? Such as #115 (comment)
To be more precise, are we keeping the multiple lint configs or are we using a single one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer keeping a single rule file and adjusting all Markdown files across all folders accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would relax the rules for non-spec markdown files, we know we need to tighten them for the spec ones (in the version folder, plus the work-in-progress file). There is no need to be fussy about blank lines around lists in files such as CONTRIBUTING which might serve as a barrier to participation - the only requirement of most of our markdown files is that they render sensibly in GitHub (we might even permit GitHub-specific markup in this context). For the specification files, these need to be much more carefully structured and be suitable for rendering with respec, so I would expect a lot more rules there.
Actually we'd like to apply the same rules to all Markdown files in this repository, including the ones in the repository root. |
MD024: false # duplicate headings | ||
MD032: false # blanks-around-lists Lists should be surrounded by blank lines | ||
MD033: false # inline HTML | ||
MD047: false # single-trailing-newline Files should end with a single newline character |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please use the same rules for all *.md
files and change the files that don't follow the rules yet.
CONFIG="" | ||
|
||
while [[ $# -gt 0 ]]; do | ||
case $1 in | ||
--config) | ||
CONFIG="$2" | ||
shift # Remove --config from processing | ||
shift # Remove the value from processing | ||
;; | ||
*) | ||
FILES+="$1 " | ||
shift # Move to the next argument | ||
;; | ||
esac | ||
done | ||
|
||
if [ -z "$CONFIG" ]; then | ||
echo "Error: --config parameter is required." | ||
exit 1 | ||
fi | ||
|
||
for filename in $FILES; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary, same rules for all files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what this is used for but I definitely don't see that we need this script, can someone explain what the problem was that this was solving? Let's try again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move back to root folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd advocate for locating a config file with a stricter set of rules here, with the all-files rules at the root of the project. We need to use the strict rules for src/overlay.md
file on the development branches as well. However I don't see any markdown files in the schemas/
directory so I'm not sure why we have put configuration there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently I didn't finish my review with comment!
MD024: false # duplicate headings | ||
MD032: false # blanks-around-lists Lists should be surrounded by blank lines | ||
MD033: false # inline HTML | ||
MD047: false # single-trailing-newline Files should end with a single newline character |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would relax the rules for non-spec markdown files, we know we need to tighten them for the spec ones (in the version folder, plus the work-in-progress file). There is no need to be fussy about blank lines around lists in files such as CONTRIBUTING which might serve as a barrier to participation - the only requirement of most of our markdown files is that they render sensibly in GitHub (we might even permit GitHub-specific markup in this context). For the specification files, these need to be much more carefully structured and be suitable for rendering with respec, so I would expect a lot more rules there.
It took me a while to come back to look at this, sorry. My suggestion:
|
Closes: #104
Here it was decided to use different markdownlint.yaml files for
/schemas
and/versions
since they have different purposes and as such, different standards.What this PR introduces:
markdownlint.yaml
exist in each folder,/versions
and/schemas
. Sub-folder linting seems to be supported by most plugins. See Differentiated config based on path DavidAnson/vscode-markdownlint#280format-markdown.sh
requires now a--config
parameter since now the config is not in a single place.package.json
to use the same namespaces.