-
Notifications
You must be signed in to change notification settings - Fork 100
Improve IndexConfig
derive macro
#616
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
base: main
Are you sure you want to change the base?
Conversation
Perhaps if there's no fields with |
IndexConfig
deriveIndexConfig
derive macro
#[proc_macro_derive(IndexConfig, attributes(index_config))] | ||
pub fn generate_index_settings(input: proc_macro::TokenStream) -> proc_macro::TokenStream { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not a maintainer)
Since this change is quite sublte otherwise: could you add a doc comment explaining how the macro can be used?
A testcase would obviously also be nice, but since this was not tested beforehand..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @funlennysub, thank you for your contribution! Giving the ability to change the index name definitely seems useful. However, I don't see what makes max_total_hits
any different than all the other settings. If we support max_total_hits
, that would leave the door open to every single other setting and I don't want this macro to become a multi-thousand line macro.
Could you just keep the way to set the index name please?
Pull Request
Related issue
None
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements: