-
Notifications
You must be signed in to change notification settings - Fork 35
schemaview.py: add get_string_type to retrieve typedef for strings #478
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
Conversation
…strings in the schema
| (nullcontext("string"), "\ntypes:\n string:\n base: str\n"), | ||
| # retrieve the string using the type URI | ||
| (nullcontext("stringiformes"), "\ntypes:\n stringiformes:\n uri: xsd:string\n base: str\n"), | ||
| # retrieve the string from the imported linkml:types "string" | ||
| (nullcontext("string"), "\nimports:\n - linkml:types\n"), |
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #478 +/- ##
==========================================
+ Coverage 78.10% 78.19% +0.09%
==========================================
Files 52 52
Lines 4512 4522 +10
Branches 983 985 +2
==========================================
+ Hits 3524 3536 +12
+ Misses 769 768 -1
+ Partials 219 218 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I wonder if we really need such a function in the public interface (that's for me what we document on the documentation webpage). Any potential future change might bring downstream breakage. We start so and we might end-up adding |
|
@ialarmedalien this PR looks pretty good to me but I think @Silvanoc has raised a valid concern. Can you please respond to that comment? |
|
This will be part of a larger function that works out what types are valid for a slot, and more specifically, it will be used to retrieve the type for a slot where the range is an enum. I'm going to add the components of the larger function in a separate feature branch. The intention is for this larger function to be used by modules that convert a LinkML schema into a different format; it can be used to remap linkml and schema-specific types into the types used in the destination format. Originally the implementation had this code integrated into the body of the function, but enums are a common slot range, and having this value cached would be helpful as it's likely to be called numerous times when running over a schema. I am happy to make the function "private" (a.k.a. rename it to |
I'm not 100% I grasp it all. But I don't think I need to 😄 I just wanted you to consider if the addition strictly needs to be public, otherwise I would make it private. If the larger function is going to become part of SchemaView, I would definitively make this function private. |
|
I've made it private. It can go public later on if it has a taste for the limelight. |
Silvanoc
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.
All concerns addressed
Note: this PR branches off #477, so that PR should ideally be approved and merged first.
Fairly simple function to find the type used for strings in a schema. I have some code in an upcoming PR that will call this repeatedly, so it's easier to create a dedicated method for it and have the result cached.