chakkk309 commented on PR #19133: URL: https://github.com/apache/datafusion/pull/19133#issuecomment-3636221009
> I think we can merge this, I'm just a bit unclear on some of the implications relating to the `cfg_attr` for tests; I don't know how they interact for example, and playing around locally with: > > ```diff > - #![cfg_attr(test, allow(clippy::needless_pass_by_value))] > + #![cfg_attr(test, expect(clippy::needless_pass_by_value))] > ``` > > I'm having a hard time understanding what exactly is the semantics here 🤔 > > Also removing some of the existing `#![cfg_attr(test, allow(clippy::needless_pass_by_value))]` in tests might make them drift with the rest of the test codebase as I assume people in the future running into this lint probably wouldn't add this exception back? I see what you mean :) , `expect` warns when the lint doesn't trigger, while `allow` stays silent. I removed those test attributes because they showed unfulfilled expectations - the lint wasn't firing there. I'd rather suppress only where it triggers, and let developers see the warning in future tests so they can decide then. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
