-
Notifications
You must be signed in to change notification settings - Fork 32
Adapt to python 3.14 #419
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?
Adapt to python 3.14 #419
Conversation
updates: - https://github.com/psf/black → https://github.com/psf/black-pre-commit-mirror - [github.com/psf/black-pre-commit-mirror: 25.1.0 → 25.11.0](psf/black-pre-commit-mirror@25.1.0...25.11.0) - [github.com/astral-sh/ruff-pre-commit: v0.12.11 → v0.14.7](astral-sh/ruff-pre-commit@v0.12.11...v0.14.7) - [github.com/pre-commit/mirrors-mypy: v1.17.1 → v1.19.0](pre-commit/mirrors-mypy@v1.17.1...v1.19.0)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #419 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 37 37
Lines 2763 2776 +13
=========================================
+ Hits 2763 2776 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
brisvag
left a comment
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.
Some comments on the latest changes. Other than that, thanks for finishing this up! I also got really stuck with the jinja stuff...
| cmd = contributions.CommandContribution(**cmd_kwargs) | ||
| if cmd.id.startswith(self.plugin_name): | ||
| n = len(self.plugin_name) | ||
| n = len(self.plugin_name) + 1 |
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.
Why is this needed?
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.
Because if plugin name is my-plugin and the contribution is my-plugin.contribution_name then
contribution[len(self.plugin_name)] gives .contribution_name that is invalid identifier, and crashes on assign validation.
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.
I mean, why is this changed from before?
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.
I do not understand. It looks like assignment validation was not triggered.
| """ | ||
|
|
||
| id: str = Field( | ||
| id: Annotated[str, AfterValidator(_validators.command_id)] = Field( |
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.
Should we then ideally migrate all our field validators to use the type annotation instead?
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.
This is suggested for reusable validators. We might want to make more validators reusable.
src/npe2/manifest/schema.py
Outdated
| if isinstance(value, list) and isinstance( | ||
| _get_inner_type(annotation), ModelMetaclass | ||
| get_inner_type(annotation), object | ||
| ): | ||
| return [check_pynames(i, (*loc, n)) for n, i in enumerate(value)] |
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.
This feels wrong. Isn't the second clause always true by definition?
Instead, we should probably check that the outer type is list, and maybe that the inner type is BaseModel or somethign like that.
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.
It is an object for the current main. Just renamed. But investigation migh be a good idea.
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.
I pushed a change that should do this a bit better!
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.
Ok now I actually pushed it. There was a bug in iter_inner_types, also fixed that.
| autogenerate: true | ||
| menus: | ||
| napari/layers/visualize: | ||
| /napari/layer_context: |
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 reasons why I had changed this to 'napari/layers/visualize' are 2:
- layer_context is not actually contributable (nor planned to be), this was a mistake in the first place
- this should not start with a backslash
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.
This change makes problems in multiple tests. Could we fix this naming in folloup PR?
| assert set(menus) == {"/napari/layer_context", "mysubmenu"} | ||
| items = list(plugin_manager.iter_menu("/napari/layer_context")) |
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.
yep, this leading backslash is wrong.
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.
Follow-up PR?
|
I added 3.14... windows not happy. |
fixed |
|
Btw I tested this in napari, and all seems to be working fine even in combination with the v1 sstuff we use there for now. |
Update npe2 dependency from napari/npe2@py314 (doesn't exist) to brisvag/npe2@py314 which contains the Python 3.14 and Pydantic V2 compatibility changes from PR napari/npe2#419. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Skip npe2 docs prep which fails due to Pydantic V2 schema incompatibility. The npe2 _docs/render.py uses .schema() and expects 'definitions' key, but Pydantic V2 returns '$defs'. Use slimfast target which uses prep-stubs instead of prep-docs to avoid cloning and running npe2's render.py. See: napari/npe2#419 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Very much work in progress... This is harder than I expected, and I expected hard xD
Suggestions are welcome, if you think I'm doing this completely wrong.
Currently there are issues with schemas failing in all sorts of funny ways...