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 validator #683

Merged
merged 25 commits into from
Mar 20, 2024
Merged

Add validator #683

merged 25 commits into from
Mar 20, 2024

Conversation

jmg-duarte
Copy link
Contributor

No description provided.

It's still a WIP, it's missing some things:
* More tests
* Check if tests cover all the branching
* Review error messages
* Better type intersection
* Even more tests, but done with quickcheck or similar
@jmg-duarte jmg-duarte requested a review from rkuhn March 5, 2024 21:35
rust/actyx/ax-aql/src/language/mod.rs Show resolved Hide resolved
rust/actyx/ax-aql/src/language/query_impl.rs Outdated Show resolved Hide resolved
rust/actyx/ax-core/src/runtime/operation/validation.rs Outdated Show resolved Hide resolved
rust/actyx/ax-core/src/runtime/operation/validation.rs Outdated Show resolved Hide resolved
rust/actyx/ax-core/src/runtime/operation/validation.rs Outdated Show resolved Hide resolved
let cbor = Cow::Owned(cbor);
if let Some(value) = value.get(&cbor) {
if let e @ Err(_) = validate(&value.decode(), ty) {
return e;
Copy link
Member

Choose a reason for hiding this comment

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

One thing I just realise:

  • if these errors are to be read by humans, they would be vastly more useful if they carried the context (i.e. the path inside the event payload object)
  • since these errors will usually not be read by humans and they are expensive to construct (string formatting), it would be really great if the error carried the information for printing but only did it in its Display impl

These two should probably be done in separate add-on PRs once this is merged.

@jmg-duarte jmg-duarte requested a review from rkuhn March 18, 2024 14:40
Copy link
Member

@rkuhn rkuhn left a comment

Choose a reason for hiding this comment

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

Yes, nearly there, just a few test upgrades in case someone ever breaks a test.

rust/actyx/ax-aql/src/language/query_impl.rs Outdated Show resolved Hide resolved
rust/actyx/ax-aql/src/language/query_impl.rs Outdated Show resolved Hide resolved
rust/actyx/ax-core/src/runtime/operation/validation.rs Outdated Show resolved Hide resolved
rust/actyx/ax-core/src/runtime/operation/validation.rs Outdated Show resolved Hide resolved
@jmg-duarte jmg-duarte merged commit 0343a68 into rku/aql-machine Mar 20, 2024
9 of 11 checks passed
@jmg-duarte jmg-duarte deleted the jmgd/aql-validator branch March 20, 2024 09:42
@github-actions github-actions bot locked and limited conversation to collaborators Mar 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants