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

Add metadata to schema #9

Merged
merged 5 commits into from
Aug 18, 2022
Merged

Add metadata to schema #9

merged 5 commits into from
Aug 18, 2022

Conversation

zargot
Copy link
Collaborator

@zargot zargot commented Aug 17, 2022

This PR adds most of the missing meta fields in the generated Cerberus schema.
PartData has also been partially rewritten, as it was a big mess.

I will cleanup the commits before merging.

@zargot zargot changed the title add metadata to schema add metadata to schema, fixes #7 Aug 17, 2022
@zargot zargot changed the title add metadata to schema, fixes #7 fixes #7: add metadata to schema Aug 17, 2022
@zargot zargot changed the title fixes #7: add metadata to schema fixes #7 Aug 17, 2022
@zargot zargot changed the title fixes #7 add metadata to schema Aug 17, 2022
@zargot zargot linked an issue Aug 17, 2022 that may be closed by this pull request
@zargot zargot marked this pull request as ready for review August 17, 2022 18:51
DougManuel
DougManuel previously approved these changes Aug 18, 2022
Copy link
Contributor

@DougManuel DougManuel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Two comments. These comments are just for discussion and consideration at this point. No need to implement in the PR.

  1. There are about 20 partTypes and we will likely have is_... for all these partTypes.

    • I have wondered if you want a more generic function such as get_partType(p, partType_name). The explicit approach you have is good and probably the best, but we'll have many of these helper functions.
    • Have names for functions and PartData more closely adhere to the definitions in the dictionary. Instead of is_cat_val(p) use is_category(p). Instead of all_rows use all_parts. Instead of att_rows use attributes, etc.
  2. We aren't ready or don't want a full OOP approach to parts because the children and dependencies are quite complicated, but partData properties could get long. It may make sense to split up partsData into a few separate objects that reflect major parts sections. Everything in the ODM dictionary is a part, but there are a few higher-order parts, including:

    • Tables - Consider taking tables out of partData and putting that into a separate object. User data is reported in tables. ODM defines key tables, but users may define additional tables if they want.
    • partType = measures, methods and attributes. User data is reported using measures, methods and attributes. Consider renaming partData into an object that describes the contents of these three main 'reportTypes'.
      User data is reported using three partTypes: measures, methods and attributes. These are high-level parts -- in OOP terms, they would be children of parts, but parents of many properties. We don't have a name to describe these three partTypes, but we need one. Maybe 'reportTypes'? Regardless, I'd consider an object that holds properties of these three types. Properties such as categorySets, categories, etc. are properties (or children) of these three partTypes.

yulric
yulric previously approved these changes Aug 18, 2022
@yulric
Copy link
Collaborator

yulric commented Aug 18, 2022

@zargot Looks good to me. Regards to Doug's comments/

  1. I don't mind the helper functions you have, but I agree that it would be nice if the names aligned more with the ODM. In addition, can we move all the magic strings into an enum for the partType, lots of repetitions.
  2. I agree that the PartData object is getting really big which could be really hard to grok for new users to the repo. Will defer to Doug for organization.

@zargot
Copy link
Collaborator Author

zargot commented Aug 18, 2022

Looks good.

Two comments. These comments are just for discussion and consideration at this point. No need to implement in the PR.

  1. There are about 20 partTypes and we will likely have is_... for all these partTypes.

    • I have wondered if you want a more generic function such as get_partType(p, partType_name). The explicit approach you have is good and probably the best, but we'll have many of these helper functions.
    • Have names for functions and PartData more closely adhere to the definitions in the dictionary. Instead of is_cat_val(p) use is_category(p). Instead of all_rows use all_parts. Instead of att_rows use attributes, etc.
  • The main reason for the is_... functions is to be able to use them in calls to filter, making the code simpler. I think it also has other benefits like abbreviation and abstraction.
  • I can rename the part stuff in a new PR 👍
  1. We aren't ready or don't want a full OOP approach to parts because the children and dependencies are quite complicated, but partData properties could get long. It may make sense to split up partsData into a few separate objects that reflect major parts sections. Everything in the ODM dictionary is a part, but there are a few higher-order parts, including:

    • Tables - Consider taking tables out of partData and putting that into a separate object. User data is reported in tables. ODM defines key tables, but users may define additional tables if they want.
    • partType = measures, methods and attributes. User data is reported using measures, methods and attributes. Consider renaming partData into an object that describes the contents of these three main 'reportTypes'.
      User data is reported using three partTypes: measures, methods and attributes. These are high-level parts -- in OOP terms, they would be children of parts, but parents of many properties. We don't have a name to describe these three partTypes, but we need one. Maybe 'reportTypes'? Regardless, I'd consider an object that holds properties of these three types. Properties such as categorySets, categories, etc. are properties (or children) of these three partTypes.

I think I have to understand this better before doing something about it. I'm guessing it will become more apparent after implementing more of the rules, etc.

Thanks 👍

@zargot
Copy link
Collaborator Author

zargot commented Aug 18, 2022

  1. I don't mind the helper functions you have, but I agree that it would be nice if the names aligned more with the ODM. In addition, can we move all the magic strings into an enum for the partType, lots of repetitions.

Good idea. 👍

@zargot zargot changed the title add metadata to schema Add metadata to schema Aug 18, 2022
@zargot zargot merged commit df15bcc into main Aug 18, 2022
@zargot zargot deleted the zargot/meta branch August 18, 2022 16:16
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.

Add meta fields to the Cerberus schema
3 participants