feat: enable adding examples #38
Conversation
alexfertel
added
priority: 3
✅ Deploy Preview for contracts-stylus canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
|
| @@ -87,7 +87,7 @@ impl Metadata { | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(test)] | |||
| #[cfg(all(test, feature = "tests"))] | |||
There was a problem hiding this comment.
do we need both all(test, feature = "tests")? I am not sure about this.
There was a problem hiding this comment.
We do! Otherwise, we would have to declare dev-dependencies used only in tests as dependencies, and they will be included in the binaries.
| license.workspace = true | ||
| repository.workspace = true | ||
| publish = false | ||
| version = "0.0.0" |
There was a problem hiding this comment.
We don't plan to version examples, since I'm expecting them to be tied to the versions of contracts, but I'm not set on this, if you have a different idea lmk!
There was a problem hiding this comment.
I think that they should follow contract's version. Just for consistency.
There was a problem hiding this comment.
Hmmm, that would add some maintenance burden, do you think it's worth it? i.e. we would have to keep versions up-to-date for all examples whenever we change the version of contracts.
There was a problem hiding this comment.
sed command should work or check in any IDE. But we can left as it is
| self.metadata.constructor(name, symbol); | ||
| } | ||
|
|
||
| pub fn decimals(&self) -> u8 { |
There was a problem hiding this comment.
Why decimals is not a part of Metadata here and you do not use #[inherit(ERC20, Metadata)]?
There was a problem hiding this comment.
Does this one answer your first question?
you do not use #[inherit(ERC20, Metadata)]
No real reason not to add it here!
There was a problem hiding this comment.
Not exactly, my question is why decimals() is not used directly from Metadata.
impl Metadata {
pub fn decimals(&self) -> u8 {
DECIMALS
}
}
There was a problem hiding this comment.
And then you need to show in examples how to use Metadata extension's functions to be accessible from Token
There was a problem hiding this comment.
Ah, that is just to showcase the way you override _decimals on consumers.
There was a problem hiding this comment.
Ok, didn't catch this...
Maybe in this case we should have a comment that this is a way to override the default impl from Metadata.
There was a problem hiding this comment.
And then you need to show in examples how to use Metadata extension's functions to be accessible from Token
Not sure I follow, wdym? If you mean that we should show how Token now has Metadata's functions, then that's what tests are for.
There was a problem hiding this comment.
Maybe in this case we should have a comment that this is a way to override the default impl from Metadata.
Ah, yeah, that makes perfect sense, I'll add it!
There was a problem hiding this comment.
Not sure I follow, wdym? If you mean that we should show how Token now has Metadata's functions, then that's what tests are for.
I missed this commit -> 4fa637e
Now it does not matter.
Main things that change:
#[cfg(erc20)]->#[cfg(feature = "erc20")].testsflag as a workaround totestis not set whencrate-typeisliborrlibrust-lang/cargo#13595alloy-primitivesdid not need to be a dependency oflib/crypto.Sorry about making so many changes at once, but they are quite small and most of the changes are needed to enable adding examples.
As a result of setting features properly, we now have a missing docs warning that we cannot fix. See alloy-rs/core#588