Skip to content
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

feat: add fragment support to entrypoints #134

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Nov 30, 2023

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
Collaborator Author

Any thoughts?

@henryiii henryiii mentioned this pull request Jan 2, 2024
@abravalheri
Copy link
Owner

abravalheri commented Jan 3, 2024

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
Copy link
Collaborator Author

henryiii commented Jan 3, 2024

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.

@henryiii
Copy link
Collaborator Author

henryiii commented Jan 3, 2024

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
Copy link
Collaborator Author

henryiii commented Jan 3, 2024

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
Copy link
Owner

abravalheri commented Jan 5, 2024

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
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
Owner

That is a good approach!

@abravalheri
Copy link
Owner

Fixed merge conflicts + updated repr method on PluginWrapper.

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

@henryiii
Copy link
Collaborator Author

henryiii commented Feb 9, 2024

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

@henryiii
Copy link
Collaborator Author

henryiii commented Feb 9, 2024

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

@henryiii
Copy link
Collaborator Author

henryiii commented Feb 9, 2024

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