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

Remove yamllint dependency #1655

Closed
wants to merge 6 commits into from
Closed

Conversation

ebeahan
Copy link
Member

@ebeahan ebeahan commented Nov 9, 2021

The yamllint dependency is only used for make yamllint, and make yammlint is not enforced in any build step (e.g., make check). Also, I suspect most folks rely on text editors for equivalent functionality, and it's unnecessary to include it as a dep.

Also adding comments noting the associated licenses for each existing dependency in the Pipfile.

@ebeahan ebeahan self-assigned this Nov 9, 2021
@ebeahan ebeahan requested a review from a team November 10, 2021 17:47
Copy link
Contributor

@djptek djptek left a comment

Choose a reason for hiding this comment

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

I ran make yamllint and am not convinced that contributors are submitting well-formatted YAML, which is a concern.

I opened an issue with options on how we might address that #1662

I also submitted a PR with fixes to the Yaml, and an alternative configuration for Yamllint
#1661

@djptek
Copy link
Contributor

djptek commented Nov 17, 2021

Thanks @ebeahan for walking me through the rationale, this is a great idea, LGTM!

@djptek djptek self-requested a review November 17, 2021 08:44
Copy link
Contributor

@djptek djptek left a comment

Choose a reason for hiding this comment

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

LGTM!

@ebeahan
Copy link
Member Author

ebeahan commented Nov 19, 2021

Going to propose a different direction in a new PR #1672.

@ebeahan ebeahan closed this Nov 19, 2021
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.

2 participants