namada
namada copied to clipboard
cli: Propagating errors from fallible `CliToSdk` conversions
Describe your changes
This PR closes #1815. I've refactored the CliToSdk trait to propagate errors (such as I/O) from fallible conversions between SDK and CLI types, this way we can avoid panics.
Indicate on which release or other PRs this topic is based on
Based on v0.31.8
Checklist before merging to draft
- [x] I have added a changelog
- [x] Git history is in acceptable state
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 61.25%. Comparing base (
61a0759) to head (7ba4802). Report is 565 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #2832 +/- ##
==========================================
+ Coverage 53.44% 61.25% +7.81%
==========================================
Files 310 290 -20
Lines 101574 88556 -13018
==========================================
- Hits 54288 54247 -41
+ Misses 47286 34309 -12977
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
hey. this is a great effort! thanks for submitting this patch!
I just have a couple of suggestions:
hey thank you for the feedback. should I commit each suggested change individually?
I generally prefer fixup! commits then a rebase, but a single commit with all the review changes is fine too
I noticed that my fork is now quite behind the main branch (48 commits behind), and some CI tests are failing after I addressed your suggested changes.
From my understanding, it's recommended to sync my fork with the main branch and rebase my changes on top of the latest commits. However, I haven't been in this position before (I'm an open source noob), so I wanted to clarify with you before proceeding. I want to ensure that I don't introduce any unintended changes or mess up the commit history. Thank you 🙏 @sug0 for your support and patience.
hey @phasewalk1. yes, CI is probably broken on your current history, since we made changes in main. it should be pretty straight forward to do a rebase on base. run the following commands:
git remote add upstream https://github.com/anoma/namada
git fetch upstream
git rebase upstream/base
if you run into conflicts, I recommend installing Sublime Merge on macOS/Windows/Linux, or Meld on Windows/Linux to resolve them.
cheers, -- sugo
@sug0 cool, no conflicts. do i just force push to my remote now?
yup
hey @tzemanovic! I saw these changes got approved, should I rebase again and update my branch?
hey @tzemanovic! I saw these changes got approved, should I rebase again and update my branch?
pls do just to get a CI run
@phasewalk1 Can you rebase this PR on the latest release v0.33? Then also make sure to run make fmt and make clippy to help the CI pass. We would like to include this in the next release. Let me know if I can help.
@brentstone I'm running into some merge conflicts that are giving me a bit of trouble. I'm a little pressed for time right now, as I have a couple math exams coming up that are taking priority. I'll have more time on Friday to fix this, but in the meantime, I can give you write access to my fork if you want to ship it sooner rather than later.
Following up on this- the conflicts are a bit pesky; I want to make sure I'm accepting the right changes. Happy to collab on this or provide write access to the fork, but as it stands I could use a little guidance. Thanks for your patience.