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.
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 JsonRef #417
Add JsonRef #417
Changes from 4 commits
953e0fa
9bfb9b2
a7cd5d3
8e34937
6694bf1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how you had the base64
.std/.url
, does it make sense to do something similar here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It very well might. I could easily be convinced. My thinking is that if I saw the following:
I wouldn't be sure what the
.string
property meant at first glance. Without a thorough understanding of thejson_ref
opcode I wouldn't immediately understand whether string refers to the key type, or the input type, or the value's decoded type.My hope was that explicitly having to set the format would make it clearer at first glance.
However, as I'm writing this, I'm thinking that named functions
.as_string
,.as_uint64
, and.as_object
would solve both issues. I'll add those 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@barnjamin I'm having second thoughts about this. As I work more with the PyTeal library, it seems that the "PyTeal way" of doing things is to use dot notation for accessing properties but passed parameters for encodings or specifying return value types. For example, you see
AssetParam.total()
to access a property of an asset param, butEcdsaVerify(EcdsaCurve.Secp256k1, *args)
for ecdsa verification.I actually think using dot notation for everything could provide a better developer experience and I would probably be in favor of adopting that method moving forward. However, if we were trying to stay consistent with existing behavior, we should get rid of
.std/.url/.as_string/etc
in favor of exported enums as function parameters.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My lightly held 2c: I'm on-board with the PR's approach. If another reviewer has a strong opinion otherwise, I can be convinced to revert for consistency.