-
Notifications
You must be signed in to change notification settings - Fork 62
feat: call .name if it's a SettingsBase + Path support #4932
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?
Changes from all commits
ee74daa
e3e6217
d08e063
13d739c
4a4c45f
e331ffc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Call .name if it's a SettingsBase + Path support |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -40,7 +40,8 @@ | |||||||||||
|
|
||||||||||||
| from __future__ import annotations | ||||||||||||
|
|
||||||||||||
| import collections | ||||||||||||
| import collections.abc | ||||||||||||
| from collections.abc import Sequence | ||||||||||||
| from contextlib import contextmanager, nullcontext | ||||||||||||
| import fnmatch | ||||||||||||
| import hashlib | ||||||||||||
|
|
@@ -61,6 +62,7 @@ | |||||||||||
| Generic, | ||||||||||||
| List, | ||||||||||||
| NewType, | ||||||||||||
| Protocol, | ||||||||||||
| Tuple, | ||||||||||||
| TypeVar, | ||||||||||||
| Union, | ||||||||||||
|
|
@@ -71,6 +73,9 @@ | |||||||||||
| import warnings | ||||||||||||
| import weakref | ||||||||||||
|
|
||||||||||||
| from typing_extensions import override | ||||||||||||
|
|
||||||||||||
| from ansys.fluent.core._types import PathType | ||||||||||||
| from ansys.fluent.core.pyfluent_warnings import ( | ||||||||||||
| PyFluentDeprecationWarning, | ||||||||||||
| PyFluentUserWarning, | ||||||||||||
|
|
@@ -673,24 +678,36 @@ def units(self) -> str | None: | |||||||||||
| return get_si_unit_for_fluent_quantity(quantity) | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| class SettingsBaseWithName(Protocol): # pylint: disable=missing-class-docstring | ||||||||||||
| __class__: type["SettingsBase"] # pyright: ignore[reportIncompatibleMethodOverride] | ||||||||||||
| name: Callable[[], str] | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| class Textual(Property): | ||||||||||||
| """Exposes attribute accessor on settings object - specific to string objects.""" | ||||||||||||
|
|
||||||||||||
| def set_state(self, state: StateT | None = None, **kwargs): | ||||||||||||
| def set_state( | ||||||||||||
| self, | ||||||||||||
| state: str | VariableDescriptor | SettingsBaseWithName | None = None, | ||||||||||||
| **kwargs, | ||||||||||||
| ): | ||||||||||||
| """Set the state of the object. | ||||||||||||
|
|
||||||||||||
| Parameters | ||||||||||||
| ---------- | ||||||||||||
| state | ||||||||||||
| Either str or VariableDescriptor. | ||||||||||||
| Either str, settings object with a ``name()`` method or VariableDescriptor. | ||||||||||||
| kwargs : Any | ||||||||||||
| Keyword arguments. | ||||||||||||
|
|
||||||||||||
| Raises | ||||||||||||
| ------ | ||||||||||||
| TypeError | ||||||||||||
| If state is not a string. | ||||||||||||
| If state is not an appropriate type. | ||||||||||||
| """ | ||||||||||||
| if isinstance(state, SettingsBase) and hasattr(state, "name"): | ||||||||||||
| state = state.name() | ||||||||||||
Gobot1234 marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+708
to
+709
|
||||||||||||
| if isinstance(state, SettingsBase) and hasattr(state, "name"): | |
| state = state.name() | |
| name_attr = getattr(state, "name", None) | |
| if callable(name_attr): | |
| state = name_attr() |
Copilot
AI
Feb 16, 2026
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.
The generic type parameter has been changed from SettingsBase[str] to SettingsBase[PathType], but this creates a type inconsistency. The get_state() method inherited from SettingsBase is typed to return StateT (which would be PathType here), but Fluent's API actually returns strings, not PathType objects. The _state_type = str on line 967 correctly indicates that Fluent stores strings internally. Consider keeping the generic parameter as SettingsBase[str] since that's what Fluent actually returns, and handle PathType conversion only in set_state() (which is already done). The type hint for set_state can still accept PathType as shown on line 970.
| class Filename(SettingsBase[PathType], Textual): | |
| class Filename(SettingsBase[str], Textual): |
Copilot
AI
Feb 16, 2026
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.
The Path support added to Filename.set_state() lacks test coverage. Consider adding a test that verifies pathlib.Path objects and other PathLike types can be passed to Filename settings objects. This would ensure the os.fspath() conversion works correctly for different PathType variants (Path objects, strings, bytes, etc.).
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.
The
Textual.set_statedocstring andRaisessection no longer match the accepted inputs: the signature now includesSettingsBaseWithName, and the runtime also acceptsVariableDescriptor, but the docs still say "Either str or VariableDescriptor" and "If state is not a string." Update the documentation/error wording to reflect the actual accepted types/behavior.