Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make lint table entries compatible with cargo [lints] #818

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

suaviloquence
Copy link
Contributor

Removes lint_name = "required-update" shorthand and renames lint-level key to level in struct entry.

In cargo's [lints] table, the level key is required (for now). We don't (in case someone wants to specify the required update and keep the default lint level), but this could cause problems if, eventually, we integrate with the lints table and cargo still keeps this behavior. Do we want to make our level key required and then relax it later?

context from zulip

@obi1kenobi
Copy link
Owner

I think it's fine if it's not required in our table and it remains required in cargo's.

If/when users migrate from our table to cargo's, they'll already be editing the manifest. If there are more than a couple of lint config lines to migrate, it should be quite easy to write a tool to automatically insert the "no-op" level setting for each lint.

I wouldn't make the ergonomics of our config table worse today to avoid a hypothetical future problem that, even if it did happen, would be relatively painless to fix.

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Excellent job on all the PRs lately. They've been super easy to review, since they are tightly scoped, well implemented and tested, and you've done a really good job providing appropriate context in the PR summary such that I have a very good idea of what I'm looking at.

Well done! It's a pleasure to have you here 🎉

@obi1kenobi obi1kenobi enabled auto-merge (squash) July 15, 2024 19:39
@obi1kenobi obi1kenobi merged commit c69a100 into obi1kenobi:main Jul 15, 2024
34 of 35 checks passed
@suaviloquence
Copy link
Contributor Author

Thank you!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants