-
Notifications
You must be signed in to change notification settings - Fork 35
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
Clippy fixes part 1 #41
Conversation
@@ -815,10 +815,10 @@ declare export class BigInt { | |||
as_int(): Int | void; | |||
|
|||
/** | |||
* @param {string} text |
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 won't cause problems with the primitive string
being used as a regular identifier from JS? I avoided using string/str/String/etc as names due to that + in rust.
What was the clippy complaint about this?
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.
We used string in other places so I was just unifying the identifiers. This wasn't a clippy thing.
However, you have a point that it's not great and we can unify all the places to use something other than string as the name
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.
@SebastienGllmt Any suggestions? A lot of these are probably going to be involved in the regeneration (not this class though I hand-wrote it) so if we can settle on one now I can update cddl-codegen to use that.
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.
Nah, I think you're right that probably calling it anything other than "string" is fine
@@ -572,7 +572,7 @@ pub enum RedeemerTagKind { | |||
} | |||
|
|||
#[wasm_bindgen] |
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.
A lot of plutus.rs
was code-generated originally but it's juts one line so it should be fine. Copy
is a bit weird since it doesn't mean much from wasm (you till get memory freeing when passed by value like we got with BigNum
iirc) and this class won't exist in rust (since we can use enums directly without needing to wrap them for wasm restrictions) after the regeneration.
@@ -163,13 +163,18 @@ impl Deserialize for TransactionUnspentOutput { | |||
} | |||
|
|||
#[wasm_bindgen] | |||
#[derive(Clone, Debug)] | |||
#[derive(Clone, Debug, Default)] |
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.
just FYI we're getting rid of these Vec-wrapper types outside of the wasm bindings in general since they were only needed due to wasm_bindgen
restrictions. Although this struct would still need to exist, we'll just be moving it to the wasm bindings crate.
string.parse::<u64>() | ||
.map_err(|e| JsError::from_str(&format! {"{:?}", e})) | ||
.map(BigNum) | ||
// have to redefine so it's visible in WASM |
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.
Won't we be getting rid of this struct in general and just be using u64
when we drop flow support?
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, but I just wanted to silence the clippy warning anyway
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 now
Once we do the regeneration, we want to make sure the code generated conforms to both
clippy
andcargo fmt
so that we don't have to disable these tools for the repo.However, that means we also have to fix clippy errors on the non-generated part of the codebase as well. This PR makes progress towards this, but definitely doesn't fix all issues (I decided to stop here because otherwise the PR would get too big and it's probably already a bit too big)