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

Make level field optional in the [lints] table #13934

Closed
wants to merge 3 commits into from

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented May 19, 2024

What does this PR try to resolve?

This PR makes the level field in the individual lint under the [lints] optional if some other key (except priority since it would make sense to have it alone) is defined. The goal is to avoid a empty lint definition.

This is to allow special configs like check-cfg in the unexpected_cfgs to omit the level field.

Users would now be able to omit the level field, resulting in:

- unexpected_cfgs = { level = "warn", check-cfg = ['cfg(has_foo)'] }
+ unexpected_cfgs = { check-cfg = ['cfg(has_foo)'] }

How should we test and review this PR?

Test putting removing the level field with/without this PR.

Reviewing this PR should be straightforward, look at the tests that change to see the effect of this change.

Additional information

#13913 (comment)

r? @epage

@rustbot rustbot added A-manifest Area: Cargo.toml issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2024
Comment on lines 2295 to 2311
if config.level().is_none() && config.config().map(|c| c.is_empty()).unwrap_or(true) {
anyhow::bail!("`lints.{tool}.{name}` missing field `level`");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the big question for me: what should be allows

Today

lint_name = {}  # disallowed
lint_name = { config_name = "config_value" }  # disallowed
lint_name = { priority = 1 }  # disallowed
lint_name = { priority = 1, config_name = "config_value" }  # disallowed
lint_name = { level = "warn" }  # allowed
lint_name = { level = "warn", priority = 1 }  # allowed

With this PR

lint_name = {}  # disallowed
lint_name = { config_name = "config_value" }  # allowed
lint_name = { priority = 1 }  # disallowed
lint_name = { priority = 1, config_name = "config_value" }  # allowed
lint_name = { level = "warn" }  # allowed
lint_name = { level = "warn", priority = 1 }  # allowed

This allows priority without level. Should we allow that?

This requires level or config, should we bother with an empty table error?

I almost lean towards either keeping the requirement on level or dropping all requirements (though priority alone feels weird).

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

This allows priority without level. Should we allow that?

I don't think so, if priority is set then level must also be set, that's a oversight on my part. Will fix it.

This requires level or config, should we bother with an empty table error?

My stance is that it is useless and weird to have empty table.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed the priority field alone without the level field. It now bail out.

@Urgau Urgau force-pushed the lints-table-optional-level branch from 658db82 to cc345e4 Compare May 20, 2024 16:16
@epage
Copy link
Contributor

epage commented May 21, 2024

We talked about this in today's Cargo team meeting and decided to defer this out.

  • We can always merge this later but we cannot undo it. We should probably learn from the current behavior works before iterating on it further.
  • There was some preference on the team that we should be explicit on the level when config is present.

@epage epage closed this May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-manifest Area: Cargo.toml issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants