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

DX: Introduce Markdownlint #7534

Merged
merged 4 commits into from
Dec 9, 2023
Merged

Conversation

Wirone
Copy link
Member

@Wirone Wirone commented Dec 8, 2023

The goal of this PR is to introduce standardised way of ensuring quality in Markdown files using markdownlint tool.

  • Run locally using docker compose run markdown-lint
  • Run in CI using dedicated workflow

@Wirone Wirone self-assigned this Dec 8, 2023
@Wirone Wirone marked this pull request as ready for review December 8, 2023 23:27
@Wirone Wirone requested a review from keradus December 8, 2023 23:27
UPGRADE-v3.md Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Tables may look more complex at first sight, but in fact these are more readable with proper spacing. PHPStorm has great markdown support and automatically aligns cells when editing, so it's easy to modify (no need to fill spaces manually in non-modified rows etc.).

@coveralls
Copy link

coveralls commented Dec 8, 2023

Coverage Status

coverage: 94.908%. remained the same
when pulling 114dce1 on Wirone:codito/lint-markdown
into 85085a7 on PHP-CS-Fixer:master.

all

# Override rules' config
rule 'MD026', :punctuation => '.,;:'
Copy link
Member Author

Choose a reason for hiding this comment

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

The difference from default config is that ! and ? are allowed (see first header of CONTRIBUTING.md).

@Wirone Wirone added topic/documentation topic/ci Github Actions, tooling labels Dec 8, 2023
It's mounted under `/fixer` because this image already has `/app` with important stuff and overwriting it makes `mdl` unavailable.
@Wirone Wirone merged commit a27d84c into PHP-CS-Fixer:master Dec 9, 2023
27 checks passed
@Wirone Wirone deleted the codito/lint-markdown branch December 9, 2023 10:26
@jorismak
Copy link

jorismak commented Dec 9, 2023

I'm confused , why is a markdown Linter in php-cs-fixer ?

@Wirone
Copy link
Member Author

Wirone commented Dec 9, 2023

Because we have markdown files in our repo and we want to keep quality there too 🤷‍♂️? As you can see in the PR's diff, there were violations that were found and fixed, now we will check it in every pipeline.

@jorismak
Copy link

jorismak commented Dec 9, 2023 via email

markdown-lint:
image: registry.gitlab.com/pipeline-components/markdownlint:latest
command: mdl --git-recurse .
working_dir: /fixer
Copy link
Member

Choose a reason for hiding this comment

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

why not /app, like everywhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

Info in the commit message 😉.

Copy link
Member

Choose a reason for hiding this comment

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

John Doe, few months from now, will not have this detail available, and it's not intuitive when looking at code only

Copy link
Member Author

Choose a reason for hiding this comment

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

I still forget that commits are squashed 🤷‍♂️. I rather work with full history. Maybe it would be better to change all volumes to /fixer and then it does not matter why it's like this?

Copy link
Member

@keradus keradus Dec 11, 2023

Choose a reason for hiding this comment

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

myself i would go this path (/fixer)

but anything that is not confusing end user will work for me

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in #7549.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/ci Github Actions, tooling topic/documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants