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 specification version field #52

Merged
merged 15 commits into from
Jul 23, 2024
Merged

Add specification version field #52

merged 15 commits into from
Jul 23, 2024

Conversation

b-wils
Copy link
Contributor

@b-wils b-wils commented Jun 5, 2024

Since v is already in use for bodymovin version, using sv here. Open to alternatives.

@b-wils b-wils linked an issue Jun 5, 2024 that may be closed by this pull request
schema/composition/animation.json Outdated Show resolved Hide resolved
schema/composition/animation.json Outdated Show resolved Hide resolved
@fmalita fmalita requested a review from kudanai June 7, 2024 14:38
@mbasaglia
Copy link
Member

I wonder if using an increasing integer is easier than a string (A tool could just use comparison instead of having to parse the version number)

@b-wils
Copy link
Contributor Author

b-wils commented Jun 11, 2024

I wonder if using an increasing integer is easier than a string (A tool could just use comparison instead of having to parse the version number)

I'm in favor.

I know JSON Schema can't really parse version strings and I can see us wanting some conditional validation down the road.

I think we would still want to track major and minor versions, so probably a field for each.

@kudanai
Copy link
Contributor

kudanai commented Jun 11, 2024

To complete the context here, lets document what the version will look like

@b-wils b-wils requested a review from fmalita June 11, 2024 18:08
Copy link
Member

@Aidosmf Aidosmf left a comment

Choose a reason for hiding this comment

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

lgtm, but let's wait for @mbasaglia ^..^

@mbasaglia
Copy link
Member

I'd probably wait to merge this until everything else for v1.0 is merged so we don't publish a schema with the version number

"title": "Specification Version",
"description": "Specification version this Lottie is targeting. This is a 6 digit number with version components encoded as `MMmmpp`, with `MM` being major version, `mm` being minor and `pp` being patch.",
"type": "integer",
"minimum": 10000
Copy link
Member

Choose a reason for hiding this comment

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

We should probably specify explicit version numbers, someone publishing a lottie with version 2.0.0 ver: 20000 would not be valid as there's no associated schema.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the key question here is if lotties created in the future with a new version should be considered invalid for older schema?

If a future lottie otherwise passes an older schema, I think that should still be good enough to be considered valid.

I am concerned about extra work to keep schemas updated in all the tooling, particularly for smaller revisions and/or platforms that have a more involved release process, such as wear.

@b-wils
Copy link
Contributor Author

b-wils commented Jun 20, 2024

I'd probably wait to merge this until everything else for v1.0 is merged so we don't publish a schema with the version number

That's a good point, we don't want different "1.0" schemas existing. We can work the version increase into the release process once we fully define it.

@b-wils b-wils added the Blocked (RFC) Objection raised that prevents the RFC from proceeding label Jun 20, 2024
@mbasaglia
Copy link
Member

Maybe we could use version 0.0 for now and change it to 1.0 before release

@b-wils
Copy link
Contributor Author

b-wils commented Jul 11, 2024

Maybe we could use version 0.0 for now and change it to 1.0 before release

Updated this to 0.1.0 for now and included it in the schema itself. Once we figure out the release process, we'll need to decide where draft specs get published and when we want to update the version number. It may be worth having an additional "isDraft" meta-field in the schema as well.

@b-wils b-wils removed the Blocked (RFC) Objection raised that prevents the RFC from proceeding label Jul 11, 2024
Players should determine what major versions they support and also any
conditional logic to handle brekaing changes across major versions as needed.
Players should expect to handle animations that specify both newer and older
versions of the Lottie specification and should issue a warning if:
Copy link
Member

Choose a reason for hiding this comment

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

the wording here is a bit awkward perhaps it can be rephrased?

also consider using upper case for SHOULD / MAY when defining compliance behaviour as per RFC 2119

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned this up a little bit. I think it's still a bit awkward but hopefully better than before. Welcome to any suggestions here.

schema/root.json Outdated
@@ -1,5 +1,6 @@
{
"$schema": "https://json-schema.org/draft/2020-12/schema",
"$id": "https://lottie.github.io/lottie-spec/specs/schema/",
"$ref": "#/$defs/composition/animation"
"$id": "https://lottie.github.io/lottie-spec/specs/schema/000100",
Copy link
Member

Choose a reason for hiding this comment

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

The URI might be different once we figure out releases, by the way I think in the file name we should use the dotted version number for readability (eg: 0.1.0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, we can update this as needed. One thing to note is that this doesn't have to be a network locator though it's more helpful if it is.

tools/schema-validate.py Outdated Show resolved Hide resolved
Comment on lines 14 to 19
* Major version updates MAY contain breaking changes that are not compatible
with previous versions of the specification.
* Minor version updates are typically adding new functionality and SHOULD NOT
contain breaking changes.
* Patch version updates are typically making minor changes to already existing
functionality.
Copy link
Member

Choose a reason for hiding this comment

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

Since I'm writing about the specs release process in the contributing file, I'll mention semantic versioning in there too

@Aidosmf Aidosmf added this to the 1.0 milestone Jul 16, 2024
@b-wils b-wils merged commit 944f29d into lottie:main Jul 23, 2024
3 checks passed
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 Spec Version to Format
6 participants