Fix config_namespace macro symbol usage#14520
Conversation
The `config_namespace` macro was relying on a few symbols being properly imported before its used. This removes that need by referring to the symbols directly with the `$crate` prefix.
datafusion/common/src/config.rs
Outdated
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests_isolated { |
There was a problem hiding this comment.
| mod tests_isolated { | |
| mod tests { |
I think it may be best to stick to convention here.
datafusion/common/src/config.rs
Outdated
| // can compile without any surrounding `use` statements. Hence putting | ||
| // it into its own test module. | ||
| #[test] | ||
| fn check_config_namespace_macro() { |
There was a problem hiding this comment.
Do you think it may be better to add this test/module in lib.rs 🤔 ?
Since the subtle addition of a use super::* in the future would effectively hide any regression here.
There was a problem hiding this comment.
I believe this location / test is exactly for ensuring macro's don't import things unnecessarily: https://github.com/apache/datafusion/tree/main/datafusion/core/tests/macro_hygiene
How about we put this test there?
datafusion/common/src/config.rs
Outdated
| // can compile without any surrounding `use` statements. Hence putting | ||
| // it into its own test module. | ||
| #[test] | ||
| fn check_config_namespace_macro() { |
There was a problem hiding this comment.
I believe this location / test is exactly for ensuring macro's don't import things unnecessarily: https://github.com/apache/datafusion/tree/main/datafusion/core/tests/macro_hygiene
How about we put this test there?
|
@alamb Thanks for the tip! I've moved the test to the macro_hygiene module. |
3 participants
The
config_namespacemacro was relying on a few symbols being properly imported before its used. This removes that need by referring to the symbols directly with the$crateprefix.Which issue does this PR close?
config_namespace!macro requiresdatafusion::common::Resultto be in scope #14518.Rationale for this change
The
config_namespace!macro relied on symbols being properly imported. This fixes that issue.What changes are included in this PR?
config_namespace!to not require imported symbols being available.#[macro_export]attribute.Are these changes tested?
Yes. There's an isolated test to show that the macro now works without any
usestatements.Are there any user-facing changes?
Some folks might get "unused import" diagnostics/warnings after this change, maybe?