-
Notifications
You must be signed in to change notification settings - Fork 56
fix: Implement migration adapter check for textual settings parameters. #4515
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?
fix: Implement migration adapter check for textual settings parameters. #4515
Conversation
The description! |
Co-authored-by: Sean Pearson <93727996+seanpearsonuk@users.noreply.github.com>
Done, Thank you |
@prmukherj Can you please add a test? |
yes, sure, will add one before merging. Thank you for mentioning it. |
@mkundu1, added the test mentioned in the fluent bug with a different case file. |
Reading the description, it’s hard to tell which parts describe the current behavior and which describe the changes introduced in this PR. This is a wider issue for us: our PR descriptions often don’t make it easy to reconstruct what changed and why. Ideally, we should aim for descriptions that make sense even years later when reading the commit history. At the moment, we’re falling short of that goal, and this PR is a good example of where we could do better. |
@seanpearsonuk, could you please have a look if it is better now? |
This change updates the type-checking logic to correctly handle cases where a migration adapter is present.
Previous Behavior
When setting a textual attribute through PyFluent, type checking was performed before delegating to the underlying Settings API.
If the provided value did not strictly match the expected type, PyFluent raised a TypeError, even in cases where a migration adapter existed to handle type conversion.
For example:
Here, the property accepts a string value, but a migration adapter is available that correctly converts a boolean (False) into the appropriate string value.
Previously, PyFluent would still raise a TypeError before this conversion could occur.
Current Behavior
If a migration adapter exists for a setting, PyFluent now bypasses type checking and directly forwards the input to the Settings API.
This allows the migration adapter to handle type conversions or other logic as intended.
Please refer to the test to see the detailed usage.
The change aligns PyFluent behavior with the Fluent-side fix (ref. bug 1293534).