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

new lint: added enum_variant_deprecated #1095

Closed
wants to merge 2 commits into from

Conversation

Gmin2
Copy link

@Gmin2 Gmin2 commented Feb 2, 2025

part of #57

@Gmin2
Copy link
Author

Gmin2 commented Feb 2, 2025

hey @obi1kenobi can yu re run the ci once? the test is passing locally

@suaviloquence
Copy link
Contributor

looks like you need to update the snapshot or you changed the test dir and didnt commit?

@obi1kenobi obi1kenobi changed the title new lint: added enum_variant_depreciated new lint: added enum_variant_deprecated Feb 3, 2025
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.

Broadly, this is moving in the right direction. It needs another pass before we can merge it, though. I think you also need to update the committed snapshot files, since they don't currently match what linting the test crates produces.

_ => { /* ... */ }
}"#
),
)
Copy link
Owner

Choose a reason for hiding this comment

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

Please configure your text editor to always add a trailing newline to all files, and ensure all files in the PR have trailing newlines.

Comment on lines +20 to +27
variant {
variant_name: name @output @tag
deprecated @filter(op: "=", value: ["$true"])
span_: span @optional {
filename @output
begin_line @output
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

This block needs a public_api_eligible check, both here and on the baseline side.

Otherwise, this lint will flag variants that were #[doc(hidden)] in the baseline, which is incorrect since they weren't public API.

Relevant docs here:
https://github.com/obi1kenobi/trustfall-rustdoc-adapter/blob/rustdoc-v39/src/rustdoc_schema.graphql#L91-L110

Please make sure to also add tests for #[doc(hidden)] enums and variants, to make sure those are never flagged by this lint.

required_update: Minor,
lint_level: Deny,
reference_link: Some("https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute"),
query: r#"
Copy link
Owner

Choose a reason for hiding this comment

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

nit: This query is a bit hard to read — it's very dense. Take a look at how other lints format queries, and try to follow their example. For example, you can use blank lines to break up blocks into smaller, more digestible chunks.

@obi1kenobi
Copy link
Owner

@Gmin2 just checking if you're still interested in driving your two PRs to completion. No worries if you're too busy with other things, I just wanted to know one way or the other.

@Gmin2
Copy link
Author

Gmin2 commented Feb 9, 2025

i was occupied with my uni exams, will able to complete this by eow only, if its urgent yu can close it

@obi1kenobi
Copy link
Owner

EOW is fine, just wanted to make sure it's still on your radar and not dropped :) Good luck with exams!

@obi1kenobi
Copy link
Owner

Going to close this for inactivity for now, just to un-claim the issue it's attached to. If you get a chance to come back to it and complete it, feel free to reopen the PR. Hope all is well!

@obi1kenobi obi1kenobi closed this Feb 23, 2025
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.

3 participants