Improve error messages when contract has missing/invalid type or missing name#255
Improve error messages when contract has missing/invalid type or missing name#255federicobond wants to merge 6 commits into
Conversation
seddonym
left a comment
seddonym
left a comment
There was a problem hiding this comment.
Thanks for looking at this - the error reporting is confusing and this is much better!
Currently tests are failing though - and generally it would be good to have test coverage for the change.
We'll also need to figure out what to do about TOML contracts.
| try: | ||
| return registry.get_contract_class(contract_options["type"]) | ||
| except NoSuchContractType as e: | ||
| raise InvalidContractOptions({"type": f"Uknown contract type '{e}'."}) |
| try: | ||
| return registry.get_contract_class(contract_options["type"]) | ||
| except NoSuchContractType as e: | ||
| raise InvalidContractOptions({"type": f"Uknown contract type '{e}'."}) |
There was a problem hiding this comment.
Probably better to use contract_options["type"] here rather than rely on the message in the exception?
| ) | ||
| for contract_options in contracts_options: | ||
| contract_class = registry.get_contract_class(contract_options["type"]) | ||
| contract_name = contract_options.get("name", contract_options['id']) |
There was a problem hiding this comment.
Unfortunately in TOML files we won't necessarily have an "id" set.
Maybe we could have a second level of fallback which just displays the index of the contract in contracts_options, e.g. "Contract 2 is not configured correctly"?
I'd be open to requiring IDs to be passed in TOML contracts, as it would be useful to be able to rely on an identifier - but that's a breaking change so let's keep it out of this PR!
There was a problem hiding this comment.
What makes it a bit harder is that we are not iterating over contracts_options but rather the filtered list, so the indices may not match what's in the file. Would it make sense to assign the index as id by default in case there is none when reading the options from the config?
There was a problem hiding this comment.
That's a good idea - yes we could assign it earlier on.
If we do this, it might be worth adding id as an optional parameter to the TOML format, and documenting it, what do you think?
There was a problem hiding this comment.
Not sure I follow, is id not supported now in the TOML format?
There was a problem hiding this comment.
In the TOML format, you can just add contracts like so (no id needed).
|
Tests should be passing now. |
2 participants
Each change is implemented in a separate commit for easier review. Let me know what you think.
When the contract type is missing from options, the error message now becomes:
When the contract type is not known or registered, the error message now becomes:
When the contract has no configured name, it will now use the contract id as a fallback: