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

refactor(schema): improve titles, connect values & remove duplications #10

Closed
wants to merge 72 commits into from

Conversation

Aidosmf
Copy link
Member

@Aidosmf Aidosmf commented Dec 3, 2021

All auto-generating Typescript tools such as https://github.com/quicktype/quicktype use title to name Interfaces or Types. Thus, it is very important how we name titles, so we could help others automate without editing. Thus, this PR is meant to improve existing titles' duplications and inconsistencies identified by running auto-generating-type tools. In addition, better titles would be more clear for readers.

Changes:

  1. rename duplication namings in different properties. e.g. the title "value" should be unique for s and v props
  2. some additional files have been created to connect reused titles for the same properties:
    2.1. /animated-properties/animated
    2.2. /helpers/property-index
    2.3. /animated-properties/expression
    2.4 /helpers/framerate
    2.5 /helpers/name
    2.6 /helpers/match-name
    2.7 /helpers/three-dimensional
    2.8 created files for each Shape ty key
  3. remove some properties if they cover under $ref
  4. add an additional prefix/suffix word into the name if it's too generic to avoid future conflicts. e.g. title "Normal"
  5. add a note about title prop importance in CONTRIBUTING.md

Suggestion: perhaps we can squash all commits before merging since there are so many of them

Copy link
Collaborator

@mbasaglia mbasaglia left a comment

Choose a reason for hiding this comment

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

Dunno, I really don't like many of the changes in here...

Replicating the names of the object / enum in my opinion just adds clutter with no real benefit.

If you generate code surely a duplicate attribute name in unrelated classes surely shouldn't cause issues...

Feels like if a generator requires unique names, the duplication of the parent title should be done by the generator, not by the schema

"default": 0
}
},
"additionalProperties": false
Copy link
Collaborator

Choose a reason for hiding this comment

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

wouldn't additionalProperties cause issues?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, to be safe and avoid unexpected problems let me remove them

@@ -7,17 +7,17 @@
{
"properties": {
"t": {
"title": "Time",
"title": "Keyframe Time",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I agree with these, it's describing the keyframe, so why repeat it in the title of the properties?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it's a generic name I wanted to create a unique title.

@@ -5,67 +5,67 @@
"description": "",
"oneOf": [
{
"title": "Normal",
"title": "Blend Mode Normal",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I definitely don't like repeating the enum name in its values to be honest

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, it's not ideal, but then can you suggest a name? so we could avoid the generic names which may be created in other props

Copy link
Collaborator

Choose a reason for hiding this comment

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

What problem would it cause if "Normal" is used elsewhere too?

Copy link
Member Author

Choose a reason for hiding this comment

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

answered in the comment below

"$schema": "https://json-schema.org/draft/2020-12/schema",
"allOf": [
{
"const": "gf",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these needed as separate constants? I think it adds complexity to the schema unnecessarily

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

dunno, seems like a lot of overhead

Copy link
Member Author

Choose a reason for hiding this comment

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

but shouldn't we create this overhead if a value/property has been used in more than one place?

"const": 3
"default": 2,
"properties": {
"lj": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why switching this from an enum to a property?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because then we can maintain the same values at one place and reuse them as a $ref. For example, I've included line-join into these 2 files:

  1. https://github.com/LottieFiles/lottie-docs/pull/10/files#diff-6842843da38497db2665d2144dd1dfb6a991303234eabfc1070497aa1872401dR8
  2. https://github.com/LottieFiles/lottie-docs/pull/10/files#diff-3cf937f1c26d726406b5d69a6f41cd0bbabe5905669c79bd070819a8d7f84291R11

but ofc I can keep only the const value in the file instead of moving the entire property.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's better to have lj in the objects and $ref pointing to only the values

"oneOf": [
{
"title": "Star",
"title": "Star Type Default",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think marking "Default" in the title is a good pattern

Copy link
Member Author

Choose a reason for hiding this comment

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

can you please suggest a better name? because "Star Type" is taken in the parent's title above

@Aidosmf
Copy link
Member Author

Aidosmf commented Dec 3, 2021

Replicating the names of the object / enum in my opinion just adds clutter with no real benefit.

Replicating the names of the parent object/enum is not ideal, please feel free to suggest better namings for generic titles.

If you generate code surely a duplicate attribute name in unrelated classes surely shouldn't cause issues...

It will not cause major issues, but the Interface and Type names will be off. For example, Normal1, Normal2, Time1, Time2, Type1, Type2 and etc. Currently, all auto-generator JSON-schema tools have a standard using the title prop for namings. Thus, if we want to achieve a single source of truth for the Lottie JSON schema - in my opinion, we should help these tools. Because if not, devs will start creating schemas in their desired languages on their own and lose connection with the lottie-docs/schema, and when we do some changes they have to sync the code manually.

The title prop is meant for bringing more information right? It doesn't really do anything for the schema validators, thus we can gain the benefit of this prop for auto-generating tools.

Look forward to your reply, thanks ^^

@mbasaglia
Copy link
Collaborator

But why would it use Normal1 / Normal2 if they are in different scopes?

I've managed to generate code without these restrictions. To me it feels more of an issue with the generator than the schema.

If two different objects have say a size attribute, it should be fine to have it called size for both rather than ellipseSize, rectangleSize.

I don't understand why the code generator wants different names for all the properties. it should only be an issue if they can be in the same object...

@Aidosmf
Copy link
Member Author

Aidosmf commented Dec 6, 2021

I've managed to generate code without these restrictions. To me it feels more of an issue with the generator than the schema.

which tool did you use? let me have a look. All tools have limitations and conversions are not perfect. But all tools use title property for guidelines.

If two different objects have say a size attribute, it should be fine to have it called size for both rather than ellipseSize, rectangleSize.

I agree but seems like these tools cannot handle complex schemas with many layers or the generators may not support a certain schema syntax.

I don't understand why the code generator wants different names for all the properties. it should only be an issue if they can be in the same object...

Yes, the tools are not intuitive enough. Some tools treat title differently:

  • Quicktype uses title only for objects and arrays. For string/number, it looks at the key name and tries to combine all description from the same property under the same scope. Which leads to losing important information and its placement. But the good thing is if it sees two identical title names the tool will not use this name at all.
  • The best generator I've found so far ishttps://github.com/bcherny/json-schema-to-typescript which supports many schema features and is more informative. It always looks at title even for a single value and allows to generate a separate Type which is more explicit but provides more information on how it was meant in the given JSON schema. But the limitation is - if a title is duplicated or has different or additional properties - the tool treats it as a separate Interface/Type and doesn't know how to combine them even it comes from a $ref with extra props.
  • other tools use title for namings and props differentiating as well

I guess all tools struggle with combining similar props because "the schemas listed in an allOf, anyOf or oneOf array know nothing of one another". Even though tools are not ideal right now for creating proper TS schema, we should treat title properly for such generator tools.

Perhaps we should have a call for further discussion.

@mbasaglia
Copy link
Collaborator

which tool did you use? let me have a look

I just parsed the JSON schema myself. I do use titles and it doesn't cause any issues, as they are unique for "classes" and the current ones work fine for property names

https://gitlab.com/mattbas/python-lottie/-/blob/master/docs/json_schema.py#L586

The code there is a bit weird because it's for adding stuff to an existing lottie object model

@Aidosmf
Copy link
Member Author

Aidosmf commented Dec 6, 2021

I'm closing the PR after our discussion. We agreed that we shouldn't update title names because it reduces human readability; the title names have been referenced in the documentation pages and they can be edited with a script before passing the schema to the auto-generator tools.

The useful commits that improve value duplications will be cherry-picked and provided in separate PRs.

@Aidosmf Aidosmf closed this Dec 6, 2021
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.

None yet

2 participants