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

Validate graphs #212

Closed
wants to merge 9 commits into from
Closed

Validate graphs #212

wants to merge 9 commits into from

Conversation

james-strauss-uwa
Copy link
Collaborator

Added a CI task to validate logical graphs in the repository against the logical graph JSON schema. The CI task fails if any graphs are invalid.

Any feedback would be appreciated.

The consequence of this is that we'd have to keep all the graphs up-to-date whenever we change the schema. It feels nice to have all the graphs in the most current format, but is it necessary?

I added tools/checkGraph.py to do the JSON validation of each file. But we may be able to replace this with a command line JSON validator (pip install json-spec ?). In that case we'd just have to recognise and skip the physical graphs, or validate them separately using the appropriate schema.

@coveralls
Copy link

coveralls commented Nov 3, 2022

Coverage Status

Coverage: 81.843% (-0.03%) from 81.87% when pulling e6b77e2 on validate-graphs into 22c8c6f on master.

Copy link
Collaborator

@pritchardn pritchardn left a comment

Choose a reason for hiding this comment

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

The changes look good to me, at some point using a command line tool would make sense.

As far as keeping these graphs in sync, it might be worth keeping a few choice graphs in the main library as examples, or keeping a separate repository of correct and maintained examples elsewhere, but perhaps this isn't necessary - I will think more about this.

I know most of these graphs are used for various tests, perhaps we could work out how to programmatically generate these cases instead, so that they are correct by default, rather than maintaining them manually which could definitely be tedious.

@james-strauss-uwa
Copy link
Collaborator Author

I know most of these graphs are used for various tests, perhaps we could work out how to programmatically generate these cases instead, so that they are correct by default, rather than maintaining them manually which could definitely be tedious.

I like this idea. We could use the JSON schema to (partially?) generate the JSON. Maybe using something like this: https://github.com/ghandic/jsf

@pritchardn
Copy link
Collaborator

I'll write up a ticket and assign to myself, hopefully I can get to this in the next week or so.

@james-strauss-uwa
Copy link
Collaborator Author

Sorry, I've already added a ticket: https://icrar.atlassian.net/browse/LIU-325

I assigned it to myself, but feel free to take it if you have time. I won't tackle it immediately

@james-strauss-uwa
Copy link
Collaborator Author

I just noticed this out-of-date PR and synced it with master.

@awicenec Could you take a look and see if this is still useful, or if it is even required?

@awicenec
Copy link
Contributor

awicenec commented Mar 2, 2023

Will merge and close this once PR220 has been accepted

@awicenec
Copy link
Contributor

awicenec commented Mar 2, 2023

This has now been merged through PR liu-338 into master, will thus close this one.

@awicenec awicenec closed this Mar 2, 2023
@awicenec awicenec deleted the validate-graphs branch March 2, 2023 10:48
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.

4 participants