Model parsing from JSON

[lixitrixi]

No description provided.

[ChrisJefferson]

I can think of two ways of implementing this kind of code, doing a parse into a json object, or making Rust objects for every type in the input, then using serde to load the whole thing into some rust objects.

I'm not saying which is better, I'm just wondering if the serde option was considered?

[ozgurakgun]

As far as I know serde would force us to use the exact same ast definition as it is in conjure, am I right? I guess having one set of data type definitions that exactly match conjure may not be a bad thing, but on the other hand we can just extract what we want out of the ast if we parse into a json object first.

[lixitrixi]

@ozgurakgun This seems to be failing rustfmt due to an error with Git; the latest commit (8960e0c) is me running cargo fmt

[lixitrixi]

As far as I know serde would force us to use the exact same ast definition as it is in conjure, am I right? I guess having one set of data type definitions that exactly match conjure may not be a bad thing, but on the other hand we can just extract what we want out of the ast if we parse into a json object first.

As I mentioned today in our quick meeting I do quite like the idea of using Serde; I will experiment with custom Serde::Serialize and Deserialize implementations, which skip over different levels of the JSON AST.

[niklasdewally]

@ozgurakgun This seems to be failing rustfmt due to an error with Git; the latest commit (8960e0c) is me running cargo fmt

It's code coverage - I've put it here #38 to fix at some point...

[ozgurakgun]

Sounds good @lixitrixi - I am curious to see the tradeoffs.

[ChrisJefferson]

One other option (although more code) would be to read into one set of structures (using serde), then translate those into the AST.

The advantage of this is it seperates correctly parsing the conjure AST, and constructing the new AST. The disadvantage is more code, and it would also tie conjure-oxide to a specific version of conjure (but I imagine that's the plan anyway).

[niklasdewally]

The previous commit is a broken build - in particular crate::error is not found for parse.rs

First, a general organisational point - conjure-oxide should maybe be a folder in the workspace, not the workspace root package. This will make cargo test run all tests across the entire project by default when run on root, and just for conjure-oxide in its directory.

[niklasdewally]

The problem is because main.rs and lib.rs are treated as separate crates by rust.

In main.rs, you did

mod parser
mod ast
use {ast::Model, parse::*};

which adds the modules parser and ast as part of the main crate. Note that in this case error is not part of the main crate as it has not been included.

A better approach is to use lib.rs to define a library, then reference this library in main.rs by the library crates external name.
That is, instead of the above, do at the top of main.rs:

use conjure_oxide::ast::Model;
use conjure_oxide::parse::*;

This is better than the first way, as you are reusing the exports and module loading in lib.rs, and not redefining this in main.rs,

LSP and cargo check probably didn't catch the error here because they assumed you are editing the lib version of the crate when you go into the problem file.

No longer relevant

[lixitrixi]

I am not able to recreate this CI error on my own... Could someone please check locally?
@niklasdewally @ozgurakgun

[niklasdewally]

oh i can reproduce the failing test case, but not the EOF sorry

[lixitrixi]

Yes, but it doesn't even reach the assert. It reaches an EOF while parsing the AST JSON from Conjure.

[niklasdewally]

Use assert_eq instead?

[niklasdewally]

@lixitrixi I tried this on palain and got the same error. I am pretty sure it's because conjure is not installed.

[niklasdewally]

Actually, there is an executable called conjure on the school host servers (and i presume on default ubuntu) - it is part of imagemagick.

Which explains why output()? didn't throw an error.

For robustness, I would check the command output status to ensure it is 0, and if it is not fail the test.

[niklasdewally]

Maybe we should store and work from the pretty json in the test runners not essence just in case conjure isn't installed for now...

Also because if you are developing on a machine without conjure tests will fail too.

[ozgurakgun]

We should depend on Conjure being installed. Depending on which one is easier: either download the latest release in the CI (and put it in a cached dir) or use podman.

Something like the following should work for example:

podman run -it --rm --network=none ghcr.io/conjure-cp/conjure:v2.5.1 conjure --help

[niklasdewally]

We should depend on Conjure being installed. Depending on which one is easier: either download the latest release in the CI (and put it in a cached dir) or use podman.

Something like the following should work for example:

podman run -it --rm --network=none ghcr.io/conjure-cp/conjure:v2.5.1 conjure --help

We are just testing Ci on Ubuntu so should be easy enough to manually fetch the tarball from GH :)

I presume we should add a conjure installation to the contribution guide in the wiki as well?

[ozgurakgun]

another resolve conflicts is needed here I think.

[lixitrixi]

@ozgurakgun This should be good to merge now!