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

Gregsdennis/markdown lint #1549

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

gregsdennis
Copy link
Member

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?

@jdesrosiers
Copy link
Member

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.

@gregsdennis
Copy link
Member Author

gregsdennis commented Oct 23, 2024

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.

@jdesrosiers
Copy link
Member

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.

@gregsdennis
Copy link
Member Author

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.

@jdesrosiers
Copy link
Member

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?

@gregsdennis
Copy link
Member Author

gregsdennis commented Oct 23, 2024

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.

@gregsdennis gregsdennis requested a review from a team November 2, 2024 00:56
@jdesrosiers
Copy link
Member

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.

@jdesrosiers
Copy link
Member

jdesrosiers commented Nov 7, 2024

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.

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.

2 participants