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

Rewrite YAML + Markdown linting, use Prettier #1457

Closed
ewels opened this issue Mar 15, 2022 · 5 comments · Fixed by #1470
Closed

Rewrite YAML + Markdown linting, use Prettier #1457

ewels opened this issue Mar 15, 2022 · 5 comments · Fixed by #1470
Assignees

Comments

@ewels
Copy link
Member

ewels commented Mar 15, 2022

YAML linting has been broken for ages, and since we fixed it recently it has now started throwing tonnes of errors everywhere and generally being annoying.

This extends to the latest pipeline sync, where PRs such as nf-core/pgdb#47 are failing CI with errors like this:

lint: info: yamllint on ..
lint: error: failed yamllint on ..
Warning: [comments] too few spaces before comment
Error: [empty-lines] too many blank lines (1 > 0)
Error: [trailing-spaces] trailing spaces
Error: [empty-lines] too many blank lines (1 > 0)
Error: [empty-lines] too many blank lines (1 > 0)
Error: [empty-lines] too many blank lines (1 > 0)
Error: [trailing-spaces] trailing spaces
Error: [indentation] wrong indentation: expected [10](https://github.com/nf-core/pgdb/runs/5560875515?check_suite_focus=true#step:4:10) but found 8
Error: [indentation] wrong indentation: expected 10 but found 8
Error: [indentation] wrong indentation: expected 10 but found 8
Error: [indentation] wrong indentation: expected 10 but found 8
Error: [indentation] wrong indentation: expected 10 but found 8
Error: [indentation] wrong indentation: expected 10 but found 8
Error: [indentation] wrong indentation: expected 10 but found 8
Error: [indentation] wrong indentation: expected 10 but found 8
Error: [indentation] wrong indentation: expected 10 but found 8
Error: [indentation] wrong indentation: expected 10 but found 8
Error: [indentation] wrong indentation: expected 10 but found 8
Error: [indentation] wrong indentation: expected 10 but found 8
Error: [indentation] wrong indentation: expected 10 but found 8

We can of course fix these errors in the template. But to be honest it feels like now is a good time to revisit this linting.

Proposal is to drop Markdown and YAML linting and instead use https://prettier.io - this does both and generally feels like a bit more of a polished tool. One major advantage is that Prettier can and does fix the linting errors for you, rather than just spitting out errors. There's also good editor plugin integrations available.

Whilst we're there, it would be good to revisit the automated PR comments. We have a weird setup for the nf-core lint results which were the result of a load of pain to do with GitHub Actions bot permissions, but the comments from the YAML and Markdown linting seem to be working now. So maybe we can simplify this a lot.

@ewels ewels self-assigned this Mar 15, 2022
@ewels ewels mentioned this issue Mar 15, 2022
4 tasks
@ewels
Copy link
Member Author

ewels commented Mar 16, 2022

But the comments from the YAML and Markdown linting seem to be working now. So maybe we can simplify this a lot.

Correction, they're still broken:

Run mshick/add-pr-comment@v1
Error: Resource not accessible by integration

I must have been looking at a PR from a branch (eg. the sync) when I thought it was working.

Ok, but still stands - just need to make it the other way around: mimic the nf-core linting comment but for the other lint tests.

@mashehu
Copy link
Contributor

mashehu commented Mar 16, 2022

#1413 (comment):

I looked a bit into switching to prettier over the weekend. I think we should stick with markdown-lint and yaml-lint for now, because most of our markdown rules (e.g. ul-indent,no-duplicate-header, etc.) can not be replicated in a prettier config as far as I can see. But if you say these rules are not that important we could switch.

@maxulysse
Copy link
Member

Can't we then run prettier as a first test and then followed by YAML and Markdown lint?

@edmundmiller
Copy link
Contributor

https://prettier.io/docs/en/options.html#print-width From our conversation about google docs in slack the other day.

@ewels ewels linked a pull request Mar 18, 2022 that will close this issue
4 tasks
@ewels
Copy link
Member Author

ewels commented Mar 18, 2022

I looked a bit into switching to prettier over the weekend. I think we should stick with markdown-lint and yaml-lint for now, because most of our markdown rules (e.g. ul-indent,no-duplicate-header, etc.) can not be replicated in a prettier config as far as I can see. But if you say these rules are not that important we could switch.

Sorry @mashehu only just seeing this now but yeah, I came to the same conclusion. But I think that those rules are not super important are were mostly to make the linter less strict. Prettier is less strict by default (whilst still being strict enough in my mind). So I think we're good, hopefully..

@ewels ewels closed this as completed Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants