-
-
Notifications
You must be signed in to change notification settings - Fork 345
Gregsdennis/markdown lint #1549
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
Conversation
the lint is still checking these files
What's checking these files? The automation that was added recently only checks the spec files. We can check markdown files intended for GitHub (like README.md) as well, but we probably don't want to use the same process because the build we have is highly customized for spec development and a lot of that doesn't apply to things like the README.
Also, I'm not sure where the PROCESS.md falls. Should it be designed for GitHub or specs? I noticed you added header ids (## Header {#header-id}
) in a couple places. That's only supported in the spec build.
When I run lint, it's checking every markdown file. It's generating warnings, not errors; nothing's failing. It's not a big deal.
The headers was from those warnings.
If we're not concerned about the GitHub markdown files, I can figure out how to ignore them locally, and we can just close this PR.
When I run lint, it's checking every markdown file.
What do you mean by "run lint"? We have npm run lint
which just checks JavaScript and we have npm run build-all
which is what builds and lints just the spec files. What are you running that's linting every markdown file?
The headers was from those warnings.
Ok, but the real question is whether we want the process doc to be a spec file or GitHub file. I think it could go either way depending on what we want to do with that document. If it's a spec file, adding the heading id is necessary. If it's a GitHub file, that's not the right fix (assuming it needs to be fixed at all). If we want it to be treated like a spec file, it should be added to build-all
so it gets linted with the spec files.
If we're not concerned about the GitHub markdown files, I can figure out how to ignore them locally, and we can just close this PR.
I wouldn't close it. I think it's still a good cleanup.
What do you mean by "run lint"?
I'm manually running npm run lint
, which seems to be checking all of the markdown files, including the github ones. (If there's a way to run it on specific files, I can't find it.)
I really have no preference on whether it checks those files or not, but it's checking them when I run it. I think it could be nice eventually to have maybe the process file up on the website, but if we do that I expect we'd want to handle that case specifically and leave the others alone.
I'm manually running
npm run lint
, which seems to be checking all of the markdown files, including the github ones. (If there's a way to run it on specific files, I can't find it.)
npm run lint
doesn't check any markdown files. It's only checking JavaScript files. I should really decouple the markdown linting from the build so it can be run independently and as part of npm run lint
as you expect it to, but that's not how it works now. You should be running npm run build-all
to build and lint spec markdown files. You'd have to run npm run build -- *.md proposals/*.md output/*.md
to get the behavior you're describing. Maybe you have some automation that somehow triggers the build command to run when you run the lint command?
Hm. Maybe I'm seeing the linting output from the build command, then.
EDIT: Confirmed that I was doing it wrong. Still, these changes are nice to have, even if they're not being published.
I'd like to wait until #1554 is merged before moving forward with this. Then we can include a .remarkrc.js
config for the Github markdown files and include that in the automation at same time as doing this cleanup.
a7c37ec
to
4ae845d
Compare
I added linting config for automation. The new build picks up more things than it did before, so there are a few more markdown linting issues to fix.
Edit: Actually it looks like it's just linting more files.
f20649a
to
b57419f
Compare
b57419f
to
668be4b
Compare
I'm going to go ahead an pull this in since it's just formatting and no content changes.
What kind of change does this PR introduce?
markdown foramtting update
Issue & Discussion References
n/a
Summary
Just applies linting to the repo markdown files. Not strictly necessary, but the lint is still checking these files, so we might as well.
Does this PR introduce a breaking change?