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 _name and _version fields to all definition files #392

Merged
merged 2 commits into from Jan 18, 2024

Conversation

magnusbaeck
Copy link
Member

Applicable Issues

Fixes #341

Description of the Change

Up to now we've been relying on the directory structure and filenames to determine the name and version of a definition file for an event or some other type. There are advantages to this, but also the drawback that it's clunky to work with and that the files always must remain in this structure, i.e. you can't just transfer the contents of a file over some channel or protocol and expect the receiver to be able to make sense of it.

The new _name and _version key are introduced in the definition file format and all existing files are updated to include them. We also add a test that makes sure that the contents of those keys matches the path to the file.

Alternate Designs

None.

Benefits

No more dependency to knowing being able to parse the filepath of a file.

Possible Drawbacks

When copying an existing file to create a new version you also need to update the _version key.

Sign-off

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or

(b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or

(c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it.

(d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved.

Signed-off-by: Magnus Bäck <magnus.back@axis.com>

Up to now we've been relying on the directory structure and filenames
to determine the name and version of a definition file for an event
or some other type. There are advantages to this, but also the drawback
that it's clunky to work with and that the files always must remain in
this structure, i.e. you can't just transfer the contents of a file over
some channel or protocol and expect the receiver to be able to make
sense of it.

The new _name and _version key are introduced in the definition file
format and all existing files are updated to include them. We also add
a test that makes sure that the contents of those keys matches the
path to the file.
@magnusbaeck magnusbaeck requested a review from a team as a code owner January 17, 2024 23:13
@e-backmark-ericsson e-backmark-ericsson dismissed their stale review January 18, 2024 12:10

Got second thoughts

@e-backmark-ericsson
Copy link
Member

I withdrew my approval to check if there could be another option to specify the event type and version in the event def file. For example by parameterizing the MetaProperty schema reference like this:

    $ref: ../EiffelMetaProperty/3.1.0.yml:EiffelActivityCanceledEvent:1.1.0

Copy link
Member

@e-backmark-ericsson e-backmark-ericsson left a comment

Choose a reason for hiding this comment

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

I didn't find any way in even the latest json schema version to parameterize the $ref statements, so I'll rest my case on that idea and instead approve this PR now

@magnusbaeck
Copy link
Member Author

I suppose we could use the $id keyword to specify the name and version of each schema.

@e-backmark-ericsson
Copy link
Member

I suppose we could use the $id keyword to specify the name and version of each schema.

Maybe. What would that look like in the yml files then? And do you see it would be transferred on to the json schema files as well?

@magnusbaeck
Copy link
Member Author

Since the YAML files are just schemas with some extra fields that we strip, we'd just start the YAML files like this:

$schema: http://json-schema.org/draft-04/schema#
$id: <some string>

However, the choice of $id string would have to be well thought out. Judging by https://stackoverflow.com/a/46682325 that URI is the base of all $ref references, so we can't just make up any URI that looks good to us. If we follow what I guess is the prevailing recommendation, namely to use the actual URI of the schema file (see e.g. https://json-schema.org/learn/getting-started-step-by-step#create-a-schema-definition) then we'd be parsing URIs instead of file paths.

Perhaps we should just go with what's in this PR?

@e-backmark-ericsson
Copy link
Member

Since the YAML files are just schemas with some extra fields that we strip, we'd just start the YAML files like this:

$schema: http://json-schema.org/draft-04/schema#
$id: <some string>

However, the choice of $id string would have to be well thought out. Judging by https://stackoverflow.com/a/46682325 that URI is the base of all $ref references, so we can't just make up any URI that looks good to us. If we follow what I guess is the prevailing recommendation, namely to use the actual URI of the schema file (see e.g. https://json-schema.org/learn/getting-started-step-by-step#create-a-schema-definition) then we'd be parsing URIs instead of file paths.

Perhaps we should just go with what's in this PR?

Yes, I vote for keeping the PR as is. My approval still stands.

@magnusbaeck magnusbaeck merged commit da53778 into eiffel-community:master Jan 18, 2024
2 checks passed
@magnusbaeck magnusbaeck deleted the nameversion branch January 18, 2024 15:39
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.

Missing Eventname and Version in the YAML files
3 participants