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

Allow extensions_options to accept Option field #14664

Merged
merged 3 commits into from
Feb 16, 2025

Conversation

goldmedal
Copy link
Contributor

Which issue does this PR close?

no related issue.

Rationale for this change

We allow users to define the custom config by extensions_options like

    extensions_options! {
        struct TestExtension {
            value: usize, default = 42
        }
    }

However, the Option type isn't allowed. It's hard to handle the null value case.

This PR allows user to define the field like

    extensions_options! {
        struct TestExtension {
            value: usize, default = 42
            option_value: Option<usize>, default = None
        }
    }

What changes are included in this PR?

  • Implement ConfigField, which has been implemented for many data types, within the macro.

Are these changes tested?

add unit test.

Are there any user-facing changes?

New behavior for extensions_options macro

@github-actions github-actions bot added common Related to common crate execution Related to the execution crate labels Feb 14, 2025
@alamb alamb added the api change Changes the API exposed to users of the crate label Feb 15, 2025
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks good to me - thank you @goldmedal

I double checked and I am pretty sure this PR is backwards compatable / has no API changes. Specifically, no existing code would need to be changed, right?

assert!(test.is_some());

assert_eq!(test.unwrap().value, 42);
assert_eq!(test.unwrap().option_value, None);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@goldmedal
Copy link
Contributor Author

I double checked and I am pretty sure this PR is backwards compatable / has no API changes. Specifically, no existing code would need to be changed, right?

Yes. no existing code needs to be changed.

@alamb alamb removed the api change Changes the API exposed to users of the crate label Feb 15, 2025
@goldmedal goldmedal merged commit 2238680 into apache:main Feb 16, 2025
24 checks passed
@goldmedal goldmedal deleted the feature/extension-option-conifg branch February 16, 2025 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Related to common crate execution Related to the execution crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants