-
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
feat: expose solidity test runner config via user config #5837
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
hardhatTotal size of the bundle: List of dependencies (sorted by size)
|
2ecb430
to
e62a664
Compare
e62a664
to
27c734f
Compare
27c734f
to
3693970
Compare
3693970
to
58fa591
Compare
58fa591
to
1c5f1a1
Compare
1c5f1a1
to
b99dd9a
Compare
b99dd9a
to
1131ca0
Compare
1131ca0
to
8d1e133
Compare
v-next/hardhat/src/internal/builtin-plugins/solidity/type-extensions.ts
Outdated
Show resolved
Hide resolved
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.
Please note, the changes to the solidity
plugin are now purely cosmetic - reducing duplication and unifying how extension points are exposed between single and multi solc versions configs.
The PR is now ready for re-review. As discussed on Slack, we'll accept the solidity test config via the |
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 small comment, but LGTM. Feel free to merge after looking at them.
@schaable I left a comment for you to.
@@ -34,6 +34,8 @@ const solcUserConfigType = z.object({ | |||
profiles: incompatibleFieldType("This field is incompatible with `version`"), | |||
}); | |||
|
|||
const singleVersionSolcUserConfigType = solcUserConfigType; |
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 don't get why this is needed.
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.
NOTE: I read the other change, and now I get that it's to match the types 1:1
. Maybe we should have a comment about it.
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.
Yes, totally, adding a comment makes perfect sense.
compilers: incompatibleFieldType( | ||
"This field is incompatible with `version`", | ||
), | ||
overrides: incompatibleFieldType( | ||
"This field is incompatible with `version`", | ||
), | ||
profiles: incompatibleFieldType( | ||
"This field is incompatible with `version`", | ||
), |
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.
Aren't these duplicated? I think they were already duplicated in the previous version though. Byt they are already present in the solcUserConfigType
.
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 think I originally duplicated it to make it explicit.
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.
Yes, I did notice that, but I didn't want to meddle with that in this PR as I felt I had already done enough of that ;) I think we could tackle that in a follow-up if we want to.
@@ -13,26 +13,33 @@ declare module "../../../types/config.js" { | |||
settings?: any; | |||
} | |||
|
|||
// eslint-disable-next-line @typescript-eslint/no-empty-interface -- This could be an extension point | |||
export interface SingleVersionSolcUserConfig extends SolcUserConfig {} |
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.
Historically, in types that we know could be extended, we favor flatter types without and duplication, as they are more flexible and robust with respect to extensions. e.g. avoiding accidentally extending more than one thing at the same time. This pattern is followed in the rest of the plugins.
Now, the way you implemented it (i.e. with interface I extends Base {}
) looks promising.
Maybe we can merge this now, and add an issue to adopt this pattern everywhere after the alpha. WDYT, @schaable ? I think we discussed reducing the duplication this at one point.
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 definitely interesting. Maybe we could create a short document comparing the two patterns to summarize their pros and cons, and then make a decision based on that.
This PR implements https://www.notion.so/nomicfoundation/Configuration-122578cdeaf58049a477dc4c585834f4
In particular, it:
test
key to thesolidity.profiles.*
user config (orsolidity.*
orsolidity
if the user uses a less granular setup than build profiles)--timeout
flag with asolidity.profiles.*.test.timeout
in user configsolidity.profiles.*.test
without exposing any EDR types