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

Enums & Literals as map keys #1178

Merged
merged 10 commits into from
Nov 19, 2024
Merged

Conversation

antoniosarosi
Copy link
Contributor

@antoniosarosi antoniosarosi commented Nov 16, 2024

Issue #1050

Added support for enums and literal strings in map keys:

enum MapKey {
  A
  B
  C
}

class Fields {
  // Enum as key
  e map<MapKey, string>
  // Single literal as key
  l1 map<"literal", string>
  // Union of literals as keys
  l2 map<"one" | "two" | ("three" | "four"), string>
}

Literal integers are more complicated since they require maps to support int keys. See #1180.


Important

Add support for enums and literal strings as map keys in BAML, with updated validation, coercion logic, and tests.

  • Behavior:
    • Support for enums and literal strings as map keys added in mod.rs and types.rs.
    • Validation logic updated to allow enums and literal strings as map keys.
    • Coercion logic updated to handle enums and literal strings as map keys.
  • Tests:
    • Added tests in map_enums_and_literals.baml and map_types.baml to verify new map key functionality.
    • Updated test_functions.py and integ-tests.test.ts to include cases for enum and literal string map keys.
  • Misc:
    • Updated error messages in error.rs to reflect new map key types.
    • Minor updates in async_client.py, sync_client.py, and client.rb to support new map key types.

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

Copy link

vercel bot commented Nov 16, 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 19, 2024 3:12am

@antoniosarosi antoniosarosi changed the title Enums as map keys Enums & Literals as map keys Nov 16, 2024
@antoniosarosi antoniosarosi marked this pull request as ready for review November 19, 2024 00:30
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 d44b23d in 1 minute and 0 seconds

More details
  • Looked at 1509 lines of code in 32 files
  • Skipped 2 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_md8Qh1rwWebIgI01


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 c8fcd92 in 36 seconds

More details
  • Looked at 692 lines of code in 14 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. integ-tests/typescript/test-report.html:260
  • Draft comment:
    The test 'should work with nested classes' is failing due to a connection error. This seems unrelated to the changes in this PR and might be an issue with the test environment or external dependencies.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test case 'should work with nested classes' is failing due to a connection error. This is likely not related to the changes in the PR, but rather an issue with the test environment or external dependencies.

Workflow ID: wflow_GV5q4zs5tFPxeeK2


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 85c172e in 17 seconds

More details
  • Looked at 37 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. integ-tests/ruby/test_functions.rb:75
  • Draft comment:
    The function name InOutLiteralStringUnionMapKey in the test does not match the function name in the BAML file. Ensure the function names are consistent across files.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_G667tOcqLEfI8rDt


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 173a1e3 in 25 seconds

More details
  • Looked at 118 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. integ-tests/typescript/test-report.html:260
  • Draft comment:
    Add a newline at the end of the file for better formatting and adherence to best practices.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The HTML file lacks a newline at the end of the file, which is a minor formatting issue but considered a best practice to include.

Workflow ID: wflow_OgAWmMWoYISlAVAF


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 3f24ec5 in 20 seconds

More details
  • Looked at 72 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. engine/baml-lib/baml/tests/validation_files/class/secure_types.baml:28
  • Draft comment:
    The updated error message correctly reflects the new validation rule that maps may have strings, enums, or literal strings as keys. Ensure this change is consistently applied across all relevant error messages.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The error message updates in the comments are consistent with the changes made in the PR to support enums and literal strings as map keys. The error messages now correctly reflect the new validation rules.

Workflow ID: wflow_uoLnvaY6RdIP9cKE


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 c7742fd in 19 seconds

More details
  • Looked at 68 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. integ-tests/typescript/test-report.html:260
  • Draft comment:
    Add a newline at the end of the file for better style consistency.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The HTML file lacks a newline at the end, which is a minor style issue but considered a best practice to fix.

Workflow ID: wflow_me2la0NtHSdZZrvQ


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

@antoniosarosi antoniosarosi added this pull request to the merge queue Nov 19, 2024
Merged via the queue into canary with commit 39e0271 Nov 19, 2024
10 checks passed
@antoniosarosi antoniosarosi deleted the antonio/enums-literals-map-keys branch November 19, 2024 03:13
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.

1 participant