feat: call .name() if it's a SettingsBase + Path support#4932
feat: call .name() if it's a SettingsBase + Path support#4932Gobot1234 wants to merge 10 commits into
Conversation
Gobot1234
requested review from
hpohekar,
mayankansys,
mkundu1,
prmukherj and
seanpearsonuk
as code owners
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce “stringly typed” usage in the settings API by allowing string-valued settings to accept a settings object reference and automatically convert it to its .name().
Changes:
- Update
Textual.set_stateto convertSettingsBaseinputs with.name()into a string before setting state. - Introduce a
SettingsBaseWithNameProtocolto reflect the new accepted input shape in type hints. - Add a
Filename.set_stateoverride intending to acceptpathlib.Pathinputs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _state_type = str | ||
|
|
||
| @override | ||
| def set_state(self, state: Path | str | None = None, **kwargs): |
There was a problem hiding this comment.
Filename.set_state now accepts a Path, but it forwards the Path directly to Textual.set_state, which only allows str/VariableDescriptor and will raise TypeError. Convert Path to str (or extend the allowed types) before calling super().set_state.
| def set_state(self, state: Path | str | None = None, **kwargs): | |
| def set_state(self, state: Path | str | None = None, **kwargs): | |
| if isinstance(state, Path): | |
| state = str(state) |
Gobot1234
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Up to standards ✅🟢 Issues
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def set_state(self, state: PathType | None = None, **kwargs): | ||
| if state is not None: | ||
| state = os.fspath(state) | ||
| return super().set_state(state, **kwargs) |
| self, | ||
| state: Sequence[str | SettingsBaseWithName | VariableDescriptor] | None = None, | ||
| **kwargs, | ||
| ): | ||
| if isinstance(state, Sequence): | ||
| state = [ | ||
| ( | ||
| entry.name() | ||
| if isinstance(entry, SettingsBase) and hasattr(entry, "name") | ||
| else entry | ||
| ) | ||
| for entry in state | ||
| ] |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Gobot1234
changed the title
Context
Currently you have to retype the name of a setting object if you reference that same object in another place. E.g. think report plots and report files. Closes #4844
Change Summary
A small check to set_state to call .name if it's a SettingsObject.
Rationale
This makes the settings api less stringly typed
Impact
Just flobject and a few tests.
Further Expansion of this could be to type the dependant relations e.g. a report file can only take a report plot object and not another settings object.