-
Notifications
You must be signed in to change notification settings - Fork 15
Typing changes #346
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?
Typing changes #346
Conversation
Fixes some static typing issues Should be easier to subclass MillingStrategy is no longer a dataclass (instead has class attributes) MillingStrategy now has class_config class attribute MIllingStrategy and MillingStrategyConfig have methods that don't need subclassing
image_settings.path is supposed to be of type Path
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.
Pull Request Overview
This PR primarily adds typing hints (e.g., Optional, TypeVar, generics) and converts many @staticmethod factories to @classmethods without changing core behavior. It also refactors UI type checks into cleaner if/elif/else blocks and updates several method signatures for clearer parameter contracts.
- Add
Optional[...],TypeVar, and generics to dataclasses and functions - Replace static factory methods with class methods and use
asdictforto_dict - Refactor UI and core function signatures (e.g.,
run_milling, widget handlers)
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| fibsem/ui/FibsemMillingWidget.py | Consolidated widget type checks and added error raising |
| fibsem/structures.py | Added typing hints, asdict, and classmethod factories |
| fibsem/milling/strategy/*.py | Introduced config_class generics and removed old init |
| fibsem/milling/patterning/plotting.py | Updated return types to include Optional and tuple |
| fibsem/milling/patterning/patterns2.py | Added generics and TypeVar for BasePattern |
| fibsem/milling/core.py | Enhanced type hints for setup_milling and imaging |
| fibsem/milling/base.py | Added generics to MillingStrategy and MillingStrategyConfig |
| fibsem/microscope.py | Changed run_milling signature to include voltage |
| fibsem/conversions.py | Refined function signatures and return type tuples |
| fibsem/alignment.py | Made alignment_current optional |
Comments suppressed due to low confidence (4)
fibsem/structures.py:418
- Spelling mistake: change 'floar' to 'float' in the error message.
f"type {type(self.height)} is unsupported for height, must be int or floar"
fibsem/conversions.py:95
- The docstring for
convert_pixels_to_metresstill refers topixels: intbut the signature now acceptsfloat. Update the docstring to match.
def convert_pixels_to_metres(pixels: float, pixelsize: float) -> float:
fibsem/ui/FibsemMillingWidget.py:521
- New error path for unsupported config types should have unit tests to ensure the TypeError is raised appropriately when an unexpected type is passed.
else:
raise TypeError(f"{strategy.name} config '{key}' is unsupported type '{type(val)}'")
fibsem/microscope.py:333
- Changing
run_millingsignature to addmilling_voltageis a breaking API change; ensure all subclass overrides and callers are updated and consider bumping the major version.
def run_milling(self, milling_current: float, milling_voltage: float, asynch: bool) -> None:
Hi @patrickcleeve2,
I find it easier to find bugs when typing is working nicely, though typing is quite annoying until Python 3.11+. I started small with fixing a few linting/typing errors and things got a bit out of hand, so I thought I'd share what I've done so far. I've tried to keep this to typing but there are a few methods I've changed to make things work for child classes, so it's a bit of a mixed bag. I shouldn't have changed any actual functionality though.
This is currently untested but I thought I'd share what I've done to see what you think before I log off for the weekend, rather than dropping some monster PR on you at some later point in time. Is this helpful, or would you prefer I don't mess with this stuff too much?