-
Notifications
You must be signed in to change notification settings - Fork 17
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: re-structure basecoin-rs
#156
Conversation
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.
Thanks @Farhad-Shabani for this PR ! 👏
Very helpful to maintain cross-project dependencies.
|
}, | ||
}; | ||
println!("{:?}", query_res); | ||
let _ = write!(std::io::stdout(), "{:#?}", query_res); |
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.
@Farhad-Shabani can you please explain why you removed println!
and writing to stdout directly?
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.
To address the upgrade_client
issue, I initially suspected that the problem might stem from the way we were parsing the output. In an attempt to gain better control over the output, I used this. However, it turned out that the issue was something else.
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.
ah. should we remove this in future PR?
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 didn't perceive much distinction between these two methods here, so I kept this. We can consider it in a later PR, if you find the println! to be more effective?
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.
they are almost the same. except println!
takes a lock on stdout. for normal purposes, it's okay to use println!
.
but my main concern was whether this was a logging statement - and if you forgot to remove 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.
I added one last comment. After that, go ahead with the merge.
Also I updated the GAIA version on CI at 11bb6af
Great work ! 🙌
Closes: #155
Along with this PR
rustfmt.toml
to leverage the Rust nightly formatter for easier imports' organization.v1.8.0