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

Linters - leave yaml's alone #1294

Merged
merged 3 commits into from
Dec 21, 2022
Merged

Conversation

omesser
Copy link
Contributor

@omesser omesser commented Dec 20, 2022

Should close: #1177

Prettier which does weird things to wrapping, it's opinionated
Specifically, here it manifested in weird wrapping of mulitline strings in the github ymls which bothered us - but to be clear, it might be doing similar things in md files or others.

I've taken a look at config options a bit, the only option I see to configure our way around this, is using --prose-wrap=preserve for yaml files - which would work 🤷 for this case, but won't protect us from long lines in the general case. An example of passing options to a prettier via restyler: https://github.com/iterative/dvc.org/blob/main/.restyled.yaml#L8
EDIT: (would be easy to do so in package.json as well)

Instead I thought it makes more sense to just have linter not touch yaml files at all - this is what this PR currently does

Also, optionally should I remove restyled config (@0x2b3bfa0 mentioned it's not used)?
it's meant to open PRs with fixes (for less technical contributors) and I'm not sure the CML team needs it (see iterative/terraform-provider-iterative#615)

@iterative/cml - you decide

@omesser omesser requested a review from a team December 20, 2022 21:55
@omesser omesser added the p2-nice-to-have Low priority label Dec 20, 2022
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

This repository hasn's used @restyled-commits in centuries. Did you mean to obliterate the .restyled.yaml file and apply your siggested patch on the following line?

"lintfix": "eslint --fix ./ && prettier --write '**/*.{js,json,md,yaml,yml}'",

@omesser
Copy link
Contributor Author

omesser commented Dec 20, 2022

This repository hasn's used @restyled-commits in centuries. Did you mean to obliterate the .restyled.yaml file and apply your siggested patch on the following line?

"lintfix": "eslint --fix ./ && prettier --write '**/*.{js,json,md,yaml,yml}'",

Yes, changing the PR @0x2b3bfa0 thanks!
I guess I read this wrong (thought it was restyler, but it was just prettier from npm run lint)

@omesser omesser changed the title Restyled - Exclude yaml files altogether Linters - leave yaml's alone Dec 20, 2022
@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Dec 20, 2022

@omesser, maybe we should also delete .restyled.yaml completely as per #1294 (review), because it's not being used. Unless I got something wrong, the Restyled GitHub App isn't even enabled for this repository. 🤖

@omesser omesser requested a review from 0x2b3bfa0 December 20, 2022 22:13
@omesser omesser temporarily deployed to external December 21, 2022 00:24 — with GitHub Actions Inactive
@omesser
Copy link
Contributor Author

omesser commented Dec 21, 2022

@0x2b3bfa0 it is done
I can add some yaml linting to "replace" the missing functionality, but... not sure it's really missing honestly. lmk preferences

Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

:shipit:

@0x2b3bfa0 0x2b3bfa0 enabled auto-merge (squash) December 21, 2022 00:25
@0x2b3bfa0
Copy link
Member

I don’t know any good solution for automatically formatting YAML workflow files, so I guess it’s fine to leave it as-is for now.

@github-actions
Copy link
Contributor

Test Comment

@github-actions
Copy link
Contributor

Test Comment

@0x2b3bfa0 0x2b3bfa0 merged commit ba6a84d into iterative:master Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Low priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

linter: update yaml formatting
2 participants