Skip to content

feat: add fragment support to entrypoints#134

Closed
henryiii wants to merge 4 commits into
abravalheri:mainfrom
henryiii:henryiii/feat/fragments
Closed

feat: add fragment support to entrypoints#134
henryiii wants to merge 4 commits into
abravalheri:mainfrom
henryiii:henryiii/feat/fragments

Conversation

@henryiii

@henryiii henryiii commented Nov 30, 2023

Copy link
Copy Markdown
Collaborator

This is an experiment in adding fragments to entrypoints. While it is suggested to use simple chars, these chars are valid according to the spec. This is the simplest way to add support for fragments to entrypoints too.

FYI, this is for validate-pyproject-schema-store, which I've got mostly setup, which would use CI to provide static copies of the schemastore files on a regular basis, and can be pinned.

@henryiii

Copy link
Copy Markdown
Collaborator Author

Any thoughts?

@henryiii henryiii mentioned this pull request Jan 2, 2024
@abravalheri

abravalheri commented Jan 3, 2024

Copy link
Copy Markdown
Owner

I was wondering, would it be a bit more conservative to use an attribute of the _load_fn that stores the fragment instead of the unorthodox entry-point naming?

For example:

# on the plugin code

def load_my_plugin(name: str):
    return json.loads(...)

load_my_plugin.fragment = "#/properties/tool/properties"

@henryiii

henryiii commented Jan 3, 2024

Copy link
Copy Markdown
Collaborator Author

Sure, I'm not a huge fan of adding attributes to functions for this, but I'm not a huge fan of the entry point naming either. :) And long-term, it might be possible that entry points could be restricted, but the attributes will always work.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii henryiii force-pushed the henryiii/feat/fragments branch from 5c2e7da to fbffdbd Compare January 3, 2024 19:49
@henryiii

henryiii commented Jan 3, 2024

Copy link
Copy Markdown
Collaborator Author

One minor downside is that every unique fragment requires a unique function. That is, you can't make a single function that takes the tool and returns the schema, you have to have one function per unique fragment (even though the functions would be identical save for this one attribute, as the fragment itself is handled by validate-pyproject).

@henryiii

henryiii commented Jan 3, 2024

Copy link
Copy Markdown
Collaborator Author

I'm okay to put this off for now if we need time to come up with a better design, by the way, since it's no longer required for any of the schemas in SchemaStore.

@abravalheri

abravalheri commented Jan 5, 2024

Copy link
Copy Markdown
Owner

One minor downside is that every unique fragment requires a unique function. That is, you can't make a single function that takes the tool and returns the schema, you have to have one function per unique fragment (even though the functions would be identical save for this one attribute, as the fragment itself is handled by validate-pyproject).

Yeah, in the scenario where we use an attribute, re-usage would require a more verbose implementation and some level of repetition... Something like:

class SchemaLoader:
   def __init__(self, fragment: str):
       self.fragment = fragment
   
   def __call__(self, name: str):
       file = importlib.resources.files(__package__).join(f"{name}.schema.json")
       return json.loads(file.read_text(encoding="utf-8"))

tool1 = SchemaLoader("/properties/tool1/properties")
tool2 = SchemaLoader("/properties/tool2/properties")
[project.entry-points."validate_pyproject.tool_schema"]
tool1 = myproject:tool1
tool2 = myproject:tool2

... which quickly become very tedious to write 😢, specially because we have to name and list the plugins in the Python code, and then again as entry-points...

Other alternatives that I can think of would also be verbose (e.g. returning a more complex object instead of a simple dictionary, with the fragment as metadata attached to the object)... Unless we introduce a different kind of entry-point that does not require listing every entry.

@henryiii

Copy link
Copy Markdown
Collaborator Author
[project.entry-points."validate_pyproject.multi_schema"]
_ = "my_plugin:get_all"

my_plugin.py

def get_all():
    return {"some#thing": ...} 

Would something like that work?

@henryiii henryiii mentioned this pull request Jan 23, 2024
@abravalheri

Copy link
Copy Markdown
Owner

That is a good approach!

@abravalheri

Copy link
Copy Markdown
Owner

Fixed merge conflicts + updated repr method on PluginWrapper.

@henryiii does this method has the same problems you mentioned in #144 (comment)?

@henryiii

henryiii commented Feb 9, 2024

Copy link
Copy Markdown
Collaborator Author

I prefer #144, which (I think, haven't tried it) would also allow us to support "extra" schemas, while this won't.

@henryiii

henryiii commented Feb 9, 2024

Copy link
Copy Markdown
Collaborator Author

(We had a baby on Wednesday so a little out of it currently ;) )

@henryiii

henryiii commented Feb 9, 2024

Copy link
Copy Markdown
Collaborator Author

The use of base.json was reverted, so --store works again, by the way (which is why our CI is working, etc). But we should probably prepare for this possibility in the future, which is better in #144, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants