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

chore: Add Markdownlint checking and CI #541

Closed
wants to merge 1 commit into from

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Oct 15, 2020

Why:

Linting the markdown will ensure that common rendering issues are caught and that the files are consistently formatted.

What's being changed:

  • Add a markdownlit config with all the current rules that are failing turned off
  • Add a CI job to run whenever markdownfiles or the config are changed
  • Update the nested code fences inside ordered list that had incorrect indenting. I would have skipped that to later, but it was crashing the parser
  • Updated the translation files for the same reason in a separate commit, but they could be undone and ignored
  • Probably easier to review with the "Hide whitespace changes" first, before looking at the rich-diffs

Check off the following:

@nschonni nschonni requested review from a team as code owners October 15, 2020 03:52
@welcome
Copy link

welcome bot commented Oct 15, 2020

Thanks for opening this pull request! A GitHub docs team member should be by to give feedback soon. In the meantime, please check out the contributing guidelines.

@nschonni
Copy link
Contributor Author

@chiedo I noticed you changed all the action version to shas. Is that something that I need to do, or would that happen if this is accepted afterwards?

@chiedo
Copy link
Contributor

chiedo commented Oct 15, 2020

👋🏿 @nschonni thanks for this PR! Yes the sha changes will automatically apply. They should be present once you click the "Update branch" button.

We're running a little behind on things but we'll review this soon!

@chiedo chiedo added the engineering Will involve Docs Engineering label Oct 15, 2020
@nschonni
Copy link
Contributor Author

@chiedo no worries, I see you have a bunch of hacktoberfest spam to deal with right now 😆

I hit the update, but since this is proposing a new Action file, I figured it might need some manual intervention

@chiedo
Copy link
Contributor

chiedo commented Oct 15, 2020

Got it! We'll take a look.

@nschonni
Copy link
Contributor Author

nschonni commented Oct 15, 2020

@chiedo aha, figured it out based on the CI failure. I copied the values from the pa11y.yml

Copy link
Contributor Author

@nschonni nschonni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased. If reviewing with the whitespace diff off, I commented on the only text change on the English doc that required changes in the translated docs because the table format issue there got mangled in the translation parser

@chiedo
Copy link
Contributor

chiedo commented Oct 19, 2020

@rachmari / @janiceilene would you be willing to take this one over? This would impact the writers much more heavily than engineering.

From an engineering perspective, the change I would suggest is that the translations directory is exempt from the linter. Because everything in that directory comes from Crowdin.

@chiedo chiedo added content This issue or pull request belongs to the Docs Content team triage Do not begin working on this issue until triaged by the team and removed engineering Will involve Docs Engineering labels Oct 19, 2020
@nschonni
Copy link
Contributor Author

@chiedo I can ignore the translations folder, or setup a separate job that you could ignore if it's failing, but I have seen Crowdin mangle formatting sometimes. EX: the tables in this PR when it misparsed the orignal english version

@chiedo
Copy link
Contributor

chiedo commented Oct 20, 2020

Thanks @nschonni ! I understand why you'd suggest that. But changes in the translations directory gets overridden regularly by a service called Crowdin. So anything issues we fixed would return with the next sync.

@nschonni
Copy link
Contributor Author

@chiedo yup, I get not wanting to take PRs directly on the content because Crowdin overwrites this (I was involved in setting it up for Node.js), but checking the content coming from Crowdin makes sure the content isn't malformed when it comes back from there.

The original parsing issue that made me need to modify the MD files in this PR has been fixed upstream, so if a release comes out there soon, I'll pull out those content changes to a new PR.

I'm thinking that this should be split into 2 jobs with 2 different path filters to handle the translations separately though. That way the second translations-only job could fail/be ignored while the translations are addressed in Crowdin, and not show a failure on other MD changes

@nschonni
Copy link
Contributor Author

@chiedo OK, split the job so there is different filtering based off of the translated vs. regular content. This way you can catch if Crowdin pushes broken markdown, but those failures won't show up on PRs for non-translated content

@nschonni
Copy link
Contributor Author

I can also rebase out the translated file changes, but the second job will crash till they are addressed

@chiedo
Copy link
Contributor

chiedo commented Oct 20, 2020

Got it thanks! We'll wait and see what @janiceilene and the writers think. This one will be their call as it would affect their workflows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engineering Will involve Docs Engineering hacktoberfest-accepted We might not merge this PR before Nov 1st, but it's a wonderful Hacktoberfest contribution! triage Do not begin working on this issue until triaged by the team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants