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

Adds more descriptive and helpful error messages to the NFT contracts and txs #233

Merged
merged 5 commits into from
Sep 16, 2024

Conversation

joshuahannan
Copy link
Member

@joshuahannan joshuahannan commented Sep 9, 2024

Closes: #232

Based on guidelines in onflow/docs#795

Description

  • Updates all error messages to include more information and relevant data about the error if possible
  • Updates CLI commands for C1.0

For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Re-reviewed Files changed in the Github PR explorer

@@ -133,7 +133,7 @@ access(all) contract ExampleNFT: NonFungibleToken {
resourceType: nil,
viewType: Type<MetadataViews.EVMBridgedMetadata>()
) as! MetadataViews.EVMBridgedMetadata?
?? panic("Could not resolve contract-level EVMBridgedMetadata")
?? panic("Could not resolve contract-level EVMBridgedMetadata.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Imho resolving a view should never
Panic. Return nil.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed! I'll remove that. Good catch!

Copy link
Collaborator

@sisyphusSmiling sisyphusSmiling left a comment

Choose a reason for hiding this comment

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

Really like the detailed error message, thanks for fixing these up! My main suggestion is not a blocker, but it would be great to see consistent formatting e.g. some messages wrap values in () or <> and others prefix values with =. In reality, this probably more of a preference/nitpick

@joshuahannan joshuahannan changed the title Adds more descriptive and helpful error messages to the contracts and txs Adds more descriptive and helpful error messages to the NFT contracts and txs Sep 10, 2024
@joshuahannan
Copy link
Member Author

@sisyphusSmiling Good point! I'll look through and make sure they are consistent. The only time I use <> is when I am dealing with types because that is usually what they are contained in in the code, so I thought it would make sense to be consistent with that. But agreed about using = vs (). I'll find a consistent format.

@joshuahannan joshuahannan merged commit 782f154 into master Sep 16, 2024
2 checks passed
@joshuahannan joshuahannan deleted the error-msgs branch September 16, 2024 21:20
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.

Update NFT Error messages to be more descriptive
3 participants