Use human friendly type format for type error display #708
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
john-h-kastner-aws
force-pushed
the
jkastner/friendly_types
branch
from
ce0154b to
eb0298b
Compare
khieta
approved these changes
Mar 8, 2024
Comment on lines
+50
to
+52
| /// Convert a custom type AST into the JSON representation of the type. | ||
| /// Conversion is done in an empty context. | ||
| pub fn custom_type_to_json_type(ty: Node<Type>) -> Result<SchemaType, ToJsonSchemaErrors> { |
There was a problem hiding this comment.
Why "custom" type? Isn't it just the internal Cedar type?
(Context: I was confused by the name at first because I was trying to figure out how it related to commonType definitions in the JSON / type declarations in the new schema syntax)
There was a problem hiding this comment.
Just copied the phrasing from another function in this file. We could perhaps do a consistency pass so that we call the new schema format one name everywhere.
Comment on lines
+2327
to
+2328
| // complete schema. TODO: the final stage of schema parsing already does | ||
| // this. Can we remove duplicated checks from human schema parsing? |
There was a problem hiding this comment.
Would be good to address this TODO in the PR, or file an issue for later & add the issue number to the comment
There was a problem hiding this comment.
I think it's not necessarily an easy change. #711
Fix #242 Signed-off-by: John Kastner <[email protected]>
Signed-off-by: John Kastner <[email protected]>
john-h-kastner-aws
force-pushed
the
jkastner/friendly_types
branch
from
eb0298b to
8cd9427
Compare
shaobo-he-aws
approved these changes
Mar 8, 2024
khieta
pushed a commit
that referenced
this pull request
Mar 11, 2024
Signed-off-by: John Kastner <[email protected]>
shaobo-he-aws
pushed a commit
that referenced
this pull request
Apr 11, 2024
Signed-off-by: John Kastner <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #242
This required a way to print out internal types that aren't accepted by the parser. I used a namespace
__cedar::internalfor this, so, for example, a type error that expects any set will say that it expectsSet<__cedar::internal::Any>.Description of changes
Issue #, if available
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
cedar-policy.I confirm that this PR (choose one, and delete the other options):
I confirm that
cedar-spec(choose one, and delete the other options):Disclaimer
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.