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 schema URL to the meta object #313

Merged
merged 10 commits into from Oct 17, 2022
Merged

Conversation

z-sztrom
Copy link
Contributor

@z-sztrom z-sztrom commented Aug 24, 2022

Applicable Issues

Fixes #280

Description of the Change

New property 'meta.schemaUri' has been added to Eiffel JSON schema.

Alternate Designs

Benefits

Particular event's structure can be verified against given JSON schema.

Possible Drawbacks

JSON schema verification takes some time and that might be critical for some applications.

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: Roman Szturc roman.szturc.ext@ericsson.com

z-sztrom and others added 2 commits August 23, 2022 13:25
Added property 'meta.schemaUri' to JSON schemas of Eiffel events. Minor
version number of all events increased accordingly.
@z-sztrom z-sztrom requested a review from a team as a code owner August 24, 2022 06:12
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.

Apart from updating the (creating new) schemas, you'd also need to update all markdown files for each event type to include this new parameter.

This PR would benefit from waiting until #282 was solved. There is a preliminary solution to that in Magnus' own clone but there might be some more work to be done there before it can be considered ready for review.

add-schema-uri.sh Outdated Show resolved Hide resolved
Added property 'meta.schemaUri' to JSON schemas of Eiffel events. Minor
version number of all events increased accordingly.
@z-sztrom z-sztrom changed the title Add schema URL to the meta object (#280) Add schema URL to the meta object Aug 31, 2022
@magnusbaeck
Copy link
Member

Yeah, waiting for #282 would make life easier for everyone; while you'd still have to create 23 new event versions the only difference in the new versions would be a bump in the meta object version reference. Plus you wouldn't have to update the documentation in 23 places, and the rebase of my patch would be less painful.

I'm finishing up the documentation but I can publish a draft PR for said issue later this week so that you'll be able to prepare a patch that's based on it.

@magnusbaeck
Copy link
Member

I've pushed the #315 draft PR that I'd recommend rebasing upon. As I said earlier you still have to create new versions of all events but the change in each file will be minimal. Apart from that you'll need to create a new version of the EiffelMetaProperty schema, found in definitions/EiffelMetaProperty. I've started documenting how things work in eiffel-syntax-and-usage/event-schemas.md and if that text doesn't make any sense please reach out.

z-sztrom and others added 2 commits October 7, 2022 12:50
Schema URL property added to EiffelMetaProperty/3.1.0.yml. JSON
schemas and corresponding MD documents generated using generate_schemas.py and
generate_docs.py respetctively.
Copy link
Member

@magnusbaeck magnusbaeck left a comment

Choose a reason for hiding this comment

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

No issues with the change itself, basically just a few merge mistakes to address.

definitions/EiffelMetaProperty/3.1.0.yml Show resolved Hide resolved
definitions/EiffelMetaProperty/3.1.0.yml Outdated Show resolved Hide resolved
Comment on lines 183 to 185
A URI pointing at a location from where the schema used when creating this
event can be retrieved. The schema on that URI should be considered
immutable.
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to hint at why or when this would be useful? I know the background of why Ericsson wants this (well, I think I do anyway) and perhaps it would make sense to include this reason here as an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extended a bit.

eiffel-vocabulary/EiffelActivityCanceledEvent.md Outdated Show resolved Hide resolved
eiffel-vocabulary/EiffelActivityTriggeredEvent.md Outdated Show resolved Hide resolved
eiffel-vocabulary/EiffelIssueDefinedEvent.md Outdated Show resolved Hide resolved
eiffel-vocabulary/EiffelTestCaseTriggeredEvent.md Outdated Show resolved Hide resolved
eiffel-vocabulary/EiffelTestSuiteStartedEvent.md Outdated Show resolved Hide resolved
For some reason not the latest files from master have been used for the
previous merge. This time it should be OK.
@z-sztrom
Copy link
Contributor Author

I don't understand why, but the merge results were really strange and I didn't notice that. My bad. Fixes submitted.

I observed strange behaviour: my local 'git pull' didn't fetch the latest versions from eiffel:master. Only when I cloned completely fresh repo, the latest versions of files appeared. I have never encountered something like that before.

@z-sztrom z-sztrom requested review from e-backmark-ericsson and magnusbaeck and removed request for e-backmark-ericsson October 11, 2022 11:38
Copy link
Member

@magnusbaeck magnusbaeck left a comment

Choose a reason for hiding this comment

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

Almost there :-)

I observed strange behaviour: my local 'git pull' didn't fetch the latest versions from eiffel:master. Only when I cloned completely fresh repo, the latest versions of files appeared. I have never encountered something like that before.

Maybe you only fetched the latest from the master branch of your fork and not the upstream remote?

definitions-bad/EiffelAnnouncementPublishedEvent/3.2.0.yml Outdated Show resolved Hide resolved
definitions/EiffelMetaProperty/3.1.0.yml Outdated Show resolved Hide resolved
magnusbaeck
magnusbaeck previously approved these changes Oct 14, 2022
Copy link
Member

@magnusbaeck magnusbaeck left a comment

Choose a reason for hiding this comment

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

Thanks!

_history:
- version: 3.2.0
introduced_in: 'No edition set'
changes: Add schema URL to the meta object (see [Issue 313](https://github.com/eiffel-community/eiffel/issues/313)).
Copy link
Member

Choose a reason for hiding this comment

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

Technically, this should be a link to the issue (280) and not the PR (313). But I can live with this and we can clean it up when updating introduced_in as we prepare the release of the Arica edition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it can help to avoid misunderstanding/confusion, I can fix it on Monday. One sed command can fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue number fixed. I hope I didn't break something else:-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protocol All protocol changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add schema url to the meta object
3 participants