fix: multi plugin id was read from the wrong place#240
Conversation
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
|
I'm not sure this was all that important, it just causes the verbose printout to be less helpful. I'm not sure the verbose printout is all that helpful with this, though: So maybe we can store the plugin name it came from too and use that. Haven't worked out why the ordering is wrong yet, though. It seems like we have a test for it. |
| @property | ||
| def id(self) -> str: | ||
| return self.schema.get("id", "MISSING ID") | ||
| return self.schema.get("$id", "MISSING ID") |
There was a problem hiding this comment.
So maybe we can store the plugin name it came from too and use that.
You mean for the sotred_plugin.id?
If that is the idea, maybe something like this?
class StoredPlugin:
def __init__(self, tool: str, schema: Schema, fallback_id: str): ...
@property
def id(self) -> str:
return self.schema.get("$id", self._fallback_id)
...
def load_from_multi_entry_point(...):
...
base_id = entry_point.name
for tool, schema in output["tools"].items():
yield StoredPlugin(tool, schema, f"{base_id}:{tool}")
for i, schema in enumerate(output.get("schemas", [])):
yield StoredPlugin("", schema, f"{base_id}:{i}")Or use base_id = f"{fn.__module__}.{fn.__name__}"
(Untested code)
Is that more or less what you had in mind in this suggestion?
There was a problem hiding this comment.
Yes, that's roughly what I was thinking. I think we use name or id in a few places, need to make sure none of those assume id is unique. If id is $id, then it is unique. Looks like your first suggestion would handle that, though. 👍
I was able to reproduce the sorting bug.
There was a problem hiding this comment.
I suppose that if the schema is not a tool, it is also reasonable to require $id to be present? (Otherwise how is the schema referenced?).
|
Thank you very much.
I suppose we can handle it separately. |
Found when working on henryiii/validate-pyproject-schema-store#117. Looks like a unit testing bug. There might be an issue with getting the ordering wrong too (it's reporting all the tools are coming from the old location, which won't allow extra schemas to load). Looking into that, might be related though.