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

Setup markdown linter #356

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

cjustinobi
Copy link

This PR implements a Markdown linter in the CI pipeline.

Changes Made

  • Created CI workflow to include filtered Markdown files.
  • Added condition to only process .md files in the root or root subdirectories.

Closes #321

Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

here is some initial feedback

MD052: true

# MD053/link-image-reference-definitions - Link and image reference definitions should be needed
MD053: true
Copy link
Member

Choose a reason for hiding this comment

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

please add a new line at the bottom to remove
image

if: steps.changed-files.outputs.files != ''
with:
config: config/.markdownlint.yaml
globs: ${{ steps.changed-files.outputs.files }}
Copy link
Member

Choose a reason for hiding this comment

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

please add a new line at the bottom to remove
image

Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

:shipit: looks good

Copy link
Collaborator

@kdeme kdeme left a comment

Choose a reason for hiding this comment

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

Did you try on your forked repo to open a PR and have this linter run?

I assume also that when it gets run there will be some errors due to these style: "consistent" restrictions. Ideally these get fixed in the same PR (or a PR right after) as adding the linter so that follow-up PRs do not fail for the wrong reasons.

name: Markdown Linter
runs-on: ubuntu-latest
steps:
- name: Checkout EIP Repository
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- name: Checkout EIP Repository
- name: Checkout portal-network-specs Repository

runs-on: ubuntu-latest
steps:
- name: Checkout EIP Repository
uses: actions/checkout@47fbe2df0ad0e27efb67a70beac3555f192b062f
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can just use actions/checkout@v4 here, not sure why EIPs repo uses that specific revision.

@cjustinobi
Copy link
Author

Did you try on your forked repo to open a PR and have this linter run?

I assume also that when it gets run there will be some errors due to these style: "consistent" restrictions. Ideally these get fixed in the same PR (or a PR right after) as adding the linter so that follow-up PRs do not fail for the wrong reasons.

I did run it, though locally with act and there was no error.

GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}

- name: Lint
uses: DavidAnson/markdownlint-cli2-action@f5cf187ef11bd3a68a127321b794aa252ff23019
Copy link
Author

Choose a reason for hiding this comment

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

Should I use version here too instead of the revision?

@cjustinobi
Copy link
Author

I did run the ci check on my forked repo as suggested, it failed against files not meeting the rules outlined in the config file, which I think is working as expected.

Screenshot 2025-01-14 at 20 41 09

@kdeme
Copy link
Collaborator

kdeme commented Jan 15, 2025

I did run the ci check on my forked repo as suggested, it failed against files not meeting the rules outlined in the config file, which I think is working as expected.

The shown error is strange though, I would not expect the line length to be checked considering this snippet from the used config:

# MD013/line-length - Line length
# Changed from default so we can allow the paragraphs to be a single line
MD013: false

@cjustinobi
Copy link
Author

I did run the ci check on my forked repo as suggested, it failed against files not meeting the rules outlined in the config file, which I think is working as expected.

The shown error is strange though, I would not expect the line length to be checked considering this snippet from the used config:

# MD013/line-length - Line length
# Changed from default so we can allow the paragraphs to be a single line
MD013: false

I think so too

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.

Set up markdown linter with rules of EIPs repo
3 participants