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

Add #[allow(unused_imports)] lint to unstable reexports #21

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

bugadani
Copy link

The example shows what the PR does - we no longer have to mark reexports with #[allow(unused_imports)].

@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 88.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 89.93%. Comparing base (cf84fbe) to head (6669567).

Files with missing lines Patch % Lines
src/item_like.rs 83.33% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #21      +/-   ##
==========================================
+ Coverage   89.72%   89.93%   +0.21%     
==========================================
  Files           4        4              
  Lines         613      626      +13     
==========================================
+ Hits          550      563      +13     
  Misses         63       63              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joshka joshka changed the title Place unused_imports on unstable reexports Add #[allow(unused_imports)] lint to unstable reexports Jan 10, 2025
- renamed impl_has_visibility to impl_item_like
- reordered matches so they flow better top to bottom
- added back whitespace (makes things a bit easier to read)
- added comments to document what's happening
- renamed allows to lin
- made the syntax for specifying allowed lints like a normal attribute
@joshka
Copy link
Member

joshka commented Jan 10, 2025

Hey there, this makes sense. Thanks

I tweaked this a bit in 0063cc1

  • renamed impl_has_visibility to impl_item_like
  • reordered matches so they flow better top to bottom
  • added back whitespace (makes things a bit easier to read)
  • added comments to document what's happening
  • renamed allows macro param to lint
  • made the syntax for specifying allowed lints like a normal attribute (#[allow(...))
  • removed the changelog entry (will be autopopulated from the commit message - I need to remove that unreleased section to make that more obv.)

Does this seem ok to you? Anything else you'd modify?

I did these changes myself rather than throwing them back your way as I wasn't sure if they'd work / be simpler and the time spent to confirm that was the same as implementing it. The comments were there from trying to understand the change.

For the squash commit mesage, something like:

Add #[allow(unused_imports)] lint to unstable reexports

Reexports marked as unstable no longer need to explicitly allow
the unused_imports lint.

Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

I'll merge and release this as-is if I don't hear from you in a few days, or sooner if you're fine with my changes.

@bugadani
Copy link
Author

bugadani commented Jan 10, 2025

Thanks, I don't think I mind the changes, except that the macro's usage looks very weird to me.

impl_item_like!(
    syn::ItemType,
    syn::ItemEnum,
    syn::ItemFn,
    syn::ItemMod,
    syn::ItemTrait,
    syn::ItemConst,
    syn::ItemStatic,
    #[allow(unused_imports)]
    syn::ItemUse,
);

This looks like the allow is on the macro's invocation, not something that is part of the syntax itself. I'm slightly worried that extending this syntax to cover other attributes may have surprising effects. If you're okay with this, I don't have objections. Thanks.

@joshka
Copy link
Member

joshka commented Jan 10, 2025

This looks like the allow is on the macro's invocation, not something that is part of the syntax itself. I'm slightly worried that extending this syntax to cover other attributes may have surprising effects. If you're okay with this, I don't have objections. Thanks.

I think that worry is a reasonable and I agree it does look a bit odd. I also thought that the change with just the square brackets looked similarly odd. I think most of this stems from the way the macro is constructed, so resolving it would be a more major rewrite. But I think for now (with the comments) the implementation is pragmatic enough. Because this isn't a user facing macro / part of the code, it can be changed in future if needed.

@joshka joshka merged commit cf3e49a into ratatui:main Jan 10, 2025
11 checks passed
@github-actions github-actions bot mentioned this pull request Jan 10, 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