-
Notifications
You must be signed in to change notification settings - Fork 0
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
Dev feature 4 - Media Types for Schemas #5
base: master
Are you sure you want to change the base?
Dev feature 4 - Media Types for Schemas #5
Conversation
on-a-t-break
commented
Sep 16, 2024
- Adds FORMAT_TYPE struct in atomicdata.hpp with name, mediatype & info fields
- Adds schematypes table with collection_name scope, schema_name key & FORMAT_TYPE vector
- Validates schema's format & format_type to ensure no mismatches
src/atomicassets.cpp
Outdated
@@ -404,6 +404,59 @@ ACTION atomicassets::extendschema( | |||
}); | |||
} | |||
|
|||
/** | |||
* Creates a new schema |
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.
Comment needs to be updated
src/atomicassets.cpp
Outdated
schema_types_t collection_schema_types = get_schema_types(collection_name); | ||
auto schema_types_itr = collection_schema_types.find(schema_name.value); | ||
|
||
check(schema_itr->format.size() == schema_format_type.size(), |
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 don't think this makes sense to enforce. Maybe I want to only set types for a subset of my attributes.
Even with this check, you could set the types first and then extend the schema afterwards, which means that you need to handle cases where types are only defined for a subset of attributes anyways.
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.
Yeah it was only setup like this to simplify checking for & matching 'mismatched' cases, but looks like it'll have to be done anyways, so yeah, no need to enforce here.
src/atomicassets.cpp
Outdated
|
||
auto & schema_format = schema_itr->format; | ||
|
||
for (auto a = 0; a < schema_format.size(); a++){ |
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 don't like that this forces the types to have to be provided in the correct order. It's not a huge problem, but especially if we support setting types for only a subset of attributes as per my comment above, this should be changed to instead iterate through the new schema_format_type elements and just check that they exist within the schema_format
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.
Implemented this new logic instead that iterates through both vectors:
// Check to see if all elements in schema_format_type have unique names && exist within the schema_format
std::set<std::string> format_type_set;
for (FORMAT_TYPE & format_type_itr : schema_format_type){
check(format_type_set.find(format_type_itr.name) == format_type_set.end(),
"Schema format type cannot contain duplicate entries");
check(std::find_if(
schema_format.begin(), schema_format.end(),
[&format_type_itr](auto & format_itr)
{ return format_type_itr.name == format_itr.name; })
!= schema_format.end(),
("No attribute in the Schema format matches the Schema format type of '" + format_type_itr.name + "'").c_str());
format_type_set.insert(format_type_itr.name);
}
@@ -37,6 +37,12 @@ namespace atomicdata { | |||
std::string type; | |||
}; | |||
|
|||
struct FORMAT_TYPE { |
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 don't really like the format type naming for this feature, because it doesn't only hold the mediatype but also the additional info. I don't have a better name in mind though.
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.
Fair! Will make a list of all things that will need "renaming" done to them
Incorporated all comments above, still need to decide on the FORMAT_TYPE struct name & then merging into CPU optimizations. |