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

Block-level constraints #1124

Merged
merged 4 commits into from
Nov 1, 2024
Merged

Block-level constraints #1124

merged 4 commits into from
Nov 1, 2024

Conversation

imalsogreg
Copy link
Contributor

@imalsogreg imalsogreg commented Oct 31, 2024

Adds the ability to specify constraints on types at the block level, like this:

class Foo {
  length_bound int
  baz string
  @@assert( valid_bound, {{ this.baz|length < this.length_bound }} )
}

TODO:

  • Integration tests for functions returning top-level block constraints
  • Integration tests for function returning classes with block-level constraints on nested classes
  • Integrations tests for function paramerts with block-level constraints
  • Integration tests for function parameters with nested block-level constraint fields
  • Documentation (Validations.mdx page and Reference)

Important

Adds block-level constraints for types, updates constraint handling logic, and includes integration tests for the new functionality.

  • Behavior:
    • Adds block-level constraints for types, allowing constraints at the class level.
    • Updates constraint handling logic in ir_helpers/mod.rs and coercer/field_type.rs.
    • Adds functions distribute_constraints, type_has_constraints, and type_has_checks in ir_helpers/mod.rs.
  • Tests:
    • Adds integration tests for block-level constraints in integ-tests/typescript/tests/integ-tests.test.ts.
    • Tests include handling of nested block-level constraints and function parameters with block-level constraints.
  • Misc:
    • Updates Cargo.toml and Cargo.lock to include itertools dependency.
    • Minor updates to BamlValueWithMeta serialization in baml_value.rs.

This description was created by Ellipsis for d21f1aa. It will automatically update as commits are pushed.

Copy link

vercel bot commented Oct 31, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
baml ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 1, 2024 10:46pm

@imalsogreg imalsogreg force-pushed the greg/block-level-constraints branch from 2d368aa to be27089 Compare October 31, 2024 18:16
@imalsogreg imalsogreg marked this pull request as ready for review October 31, 2024 19:28
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 1cbb5d0 in 2 minutes and 4 seconds

More details
  • Looked at 4207 lines of code in 45 files
  • Skipped 1 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. engine/baml-lib/jsonish/src/tests/mod.rs:134
  • Draft comment:
    Ensure that ir.distribute_constraints is used instead of FieldType::distribute_constraints as the latter has been removed. This applies to other instances where FieldType::distribute_constraints was used.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. engine/baml-lib/baml-types/src/field_type/mod.rs:158
  • Draft comment:
    The comment about distribute_constraints is outdated since the function is removed. Consider removing or updating this comment to avoid confusion.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The function distribute_constraints is implemented twice, once in IRHelper and once in FieldType. The implementation in FieldType is removed, but the comment explaining its purpose is still present. This could be confusing for future developers.
3. engine/baml-lib/baml-core/src/ir/ir_helpers/mod.rs:13
  • Draft comment:
    The function distribute_constraints is implemented twice, once in IRHelper and once in FieldType. The implementation in FieldType is removed, but the function is still called in some places. Ensure all calls are updated to use the IRHelper implementation to avoid runtime errors.
  • Reason this comment was not posted:
    Marked as duplicate.
4. engine/baml-lib/baml-core/src/ir/ir_helpers/mod.rs:377
  • Draft comment:
    The distribute_constraints function does not handle FieldType::Tuple. Consider adding a case for it to ensure all field types are covered.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The distribute_constraints function in baml-lib/baml-core/src/ir/ir_helpers/mod.rs is missing a check for FieldType::Tuple. This could lead to unexpected behavior if tuples are used with constraints.

Workflow ID: wflow_6rxfaNbBmxbx0S3U


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@imalsogreg imalsogreg force-pushed the greg/block-level-constraints branch from 45923d8 to daf1646 Compare October 31, 2024 22:27
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on daf1646 in 1 minute and 1 seconds

More details
  • Looked at 2927 lines of code in 44 files
  • Skipped 1 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. integ-tests/typescript/tests/integ-tests.test.ts:746
  • Draft comment:
    Consider adding a test case for a successful block-level check scenario to ensure both success and failure cases are covered.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test case for 'constraints: should handle block-level checks' is correctly checking the status of the 'cross_field' check. However, it would be beneficial to add a test case for a successful scenario as well.
2. integ-tests/typescript/tests/integ-tests.test.ts:751
  • Draft comment:
    Consider adding a test case for a failed nested-block-level check scenario to ensure both success and failure cases are covered.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test case for 'constraints: should handle nested-block-level checks' is correctly checking the status of the 'cross_field' check. However, it would be beneficial to add a test case for a failure scenario as well.

Workflow ID: wflow_eh9Ps3Cpq8EZibSX


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on c470414 in 35 seconds

More details
  • Looked at 96 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. fern/03-reference/baml/attributes/check.mdx:15
  • Draft comment:
    Add a BAML language identifier to the code block for consistency with other examples.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The documentation update in validations.mdx and assert.mdx files is consistent with the new feature of block-level constraints. However, the check.mdx file also needs a similar update to reflect block-level checks.

Workflow ID: wflow_g7QflJs31pNzoCQu


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on d21f1aa in 41 seconds

More details
  • Looked at 20 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. engine/baml-lib/parser-database/src/attributes/constraint.rs:24
  • Draft comment:
    Consider providing a more descriptive error message for invalid attribute names to aid debugging.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The error message for invalid attribute names is being pushed to the context, which is a good practice for error handling. However, the message could be more descriptive to aid debugging.

Workflow ID: wflow_wV7hvrHPbUWE15jx


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@imalsogreg imalsogreg added this pull request to the merge queue Nov 1, 2024
Merged via the queue into canary with commit e931acb Nov 1, 2024
9 of 10 checks passed
@imalsogreg imalsogreg deleted the greg/block-level-constraints branch November 1, 2024 22:42
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.

2 participants