chakkk309 commented on PR #19133: URL: https://github.com/apache/datafusion/pull/19133#issuecomment-3639487817
> > > 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. > > > > > > I think it was a conscious decision to use `allow` instead of `expect` for this test wide configuration as we didn't want this lint applied to test code, both for present code and future code. I feel its more correct to have it `allow` since we're then essentially turning off the lint, otherwise with `expect` it's as if we expect the test code to trigger the lint. > > So how about we change it back to `allow` and add a comment explaining the rationale? Thanks for the feedback! I've changed it back to allow and added a comment explaining that we use allow instead of expect for test configuration to explicitly disable the lint for all test code rather than expecting violations. -- 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]
