-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Type-safe HardhatError
#5283
Type-safe HardhatError
#5283
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
a495ad9
to
cc7b887
Compare
|
||
template = template.replaceAll(variableTag, value); | ||
} | ||
if (Array.isArray(rawValue)) { |
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.
@schaable when merging this conflict I realized that the hasOwnProperty
method didn't allow for classes to define their toString
, so I switched it to explicitly handling arrays.
It should be easiear to review this one commit at the time. The 4th and 6th commits are just automatic string replacements. |
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 left a few questions for my own understanding, but pre-approving.
string, | ||
ErrorMessageTemplateValue | ||
>; | ||
export type MessagetTemplateArguments<MessageTemplateT extends string> = |
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.
really nice 💯
for (const variableName of Object.keys(values)) { | ||
let value: string; | ||
|
||
return template.replaceAll(/{(.*?)}/g, (_match, variableName) => { |
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.
should this match the empty group {}
?
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.
Good catch! But after giving it a few minutes, I think it's better if it matches the behavior of the types. To avoid matching the empty group, the types will be significantly more complex.
I'll add a test for this case.
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.
Added the tests in 409dc51
HardhatError.ERRORS.INTERNAL.ASSERTION_ERROR, | ||
); | ||
}); | ||
describe("MessagetTemplateArguments type", () => { |
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.
this is a nice example of type testing
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.
LGTM, just a question regarding the regex
Co-authored-by: John Kane <[email protected]>
b01ff52
to
409dc51
Compare
This PR modifies
HardhatError
so that its constructor and itsmessageArguments
property are type-safe.This introduces two extra changes:
{variable}
instead of%variable%
, as that dramatically simplified this PR.