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

feat: allow optional lists and maps #1251

Merged
merged 6 commits into from
Dec 22, 2024

Conversation

revidious
Copy link
Contributor

@revidious revidious commented Dec 17, 2024

Allow optional lists and maps

Resolves #948

Description

This PR adds support for optional lists and maps in BAML, allowing users to declare properties like string[]? and map<string, int>?. Previously these types were not supported, and the parser would coerce unset parameters into empty lists/maps.

Changes

  1. Parser (parse_types.rs):
    • Added support for optional arrays with string[]? syntax
  • Added optional list and map support using syntax of map<string, int>?
    • Plenty of test cases covered, both optional list and optional map
  1. Json Schema Generation(json_schema.rs)
    • Supporting the optional array and maps in the generation
    • Correct JSON schema is generated in, with
      • appropriate type definitions([\"array\", \"null\"], or [\"object\",\"null\"])
      • Null default values
  • Documentation
  1. Grammar (datamodel.pest):
    • Changed array notation to allow optional token
    • Changed map syntax to allow optional token

Example Usage

class Foo {
    optional_array string[]?
    optional_map map<string, int>?
}

Testing

  • Added parser tests for optional arrays and maps
  • Verified JSON schema generation
  • Tested with various type combinations

Important

Adds support for optional lists and maps in BAML, updating parser, JSON schema generation, and grammar to handle string[]? and map<string, int>? syntax.

  • Behavior:
    • Support for optional lists and maps added in parse_types.rs using string[]? and map<string, int>? syntax.
    • Parser now distinguishes between required and optional types, updating FieldArity accordingly.
    • JSON schema generation in json_schema.rs updated to handle optional arrays and maps, allowing null values.
  • Grammar:
    • Updated datamodel.pest to allow optional token for arrays and maps.
  • Testing:
    • Added tests for optional arrays and maps in parse_types.rs to verify correct parsing and schema generation.

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

Resolves BoundaryML#948

- Added support for optional arrays with string[]? syntax\n- Added support for optional maps with map<string, int>? syntax\n- Updated JSON schema generation for optional types\n- Added comprehensive test cases
Copy link

vercel bot commented Dec 17, 2024

@revidious is attempting to deploy a commit to the Gloo Team on Vercel.

A member of the Team first needs to authorize it.

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 8a31ab9 in 20 seconds

More details
  • Looked at 306 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. engine/baml-lib/baml-core/src/ir/json_schema.rs:235
  • Draft comment:
    The required field is being populated with optional fields instead of required ones. It should be populated with fields that are not optional.
if let FieldType::Optional(_) = t {
    // Do nothing for optional fields
} else {
    required_props.push(name.clone());
}
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_hG9ygX9vO60xfENi


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

@aaronvg
Copy link
Contributor

aaronvg commented Dec 17, 2024

nice work on all these PRs! This one LGTM, will ping someone else on the team to also take a look.

@antoniosarosi antoniosarosi self-assigned this Dec 17, 2024
@antoniosarosi antoniosarosi self-requested a review December 19, 2024 17:56
@antoniosarosi antoniosarosi removed their assignment Dec 19, 2024
Copy link
Contributor

@antoniosarosi antoniosarosi left a comment

Choose a reason for hiding this comment

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

@revidious LGTM! I made some changes to your PR, mainly removed the "Maps/Lists are not allowed to be optional" error, added validation and integ tests for the new feature and then I made some changes to the spelling in the docs you wrote because it looks like you used some kind of automatic spell-checker that messes up the punctuation and capitalization.

@hellovai hellovai added this pull request to the merge queue Dec 22, 2024
Merged via the queue into BoundaryML:canary with commit 9170b89 Dec 22, 2024
6 of 7 checks passed
@revidious revidious deleted the feature/optional-lists-maps branch December 23, 2024 20:00
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.

allow optional lists and maps
4 participants