namada icon indicating copy to clipboard operation
namada copied to clipboard

cli: Propagating errors from fallible `CliToSdk` conversions

Open phasewalk1 opened this issue 1 year ago • 12 comments

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

phasewalk1 avatar Mar 07 '24 01:03 phasewalk1

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.

codecov[bot] avatar Mar 07 '24 02:03 codecov[bot]

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?

phasewalk1 avatar Mar 07 '24 09:03 phasewalk1

I generally prefer fixup! commits then a rebase, but a single commit with all the review changes is fine too

sug0 avatar Mar 07 '24 10:03 sug0

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.

phasewalk1 avatar Mar 09 '24 01:03 phasewalk1

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 avatar Mar 09 '24 07:03 sug0

@sug0 cool, no conflicts. do i just force push to my remote now?

phasewalk1 avatar Mar 09 '24 08:03 phasewalk1

yup

sug0 avatar Mar 09 '24 09:03 sug0

hey @tzemanovic! I saw these changes got approved, should I rebase again and update my branch?

phasewalk1 avatar Mar 25 '24 17:03 phasewalk1

hey @tzemanovic! I saw these changes got approved, should I rebase again and update my branch?

pls do just to get a CI run

tzemanovic avatar Mar 28 '24 14:03 tzemanovic

@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 avatar Apr 15 '24 19:04 brentstone

@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.

phasewalk1 avatar Apr 15 '24 20:04 phasewalk1

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.

phasewalk1 avatar Apr 23 '24 17:04 phasewalk1