Skip to content
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

cli: Propagating errors from fallible CliToSdk conversions #2832

Merged

Conversation

phasewalk1
Copy link
Contributor

@phasewalk1 phasewalk1 commented Mar 7, 2024

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

  • I have added a changelog
  • Git history is in acceptable state

Copy link

codecov bot commented Mar 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@phasewalk1 phasewalk1 marked this pull request as ready for review March 7, 2024 02:35
Copy link
Collaborator

@sug0 sug0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey. this is a great effort! thanks for submitting this patch!

I just have a couple of suggestions:

crates/apps/src/lib/cli.rs Outdated Show resolved Hide resolved
crates/apps/src/lib/cli.rs Outdated Show resolved Hide resolved
crates/apps/src/lib/cli.rs Outdated Show resolved Hide resolved
crates/apps/src/lib/cli.rs Outdated Show resolved Hide resolved
crates/apps/src/lib/cli.rs Outdated Show resolved Hide resolved
@phasewalk1
Copy link
Contributor Author

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?

@sug0
Copy link
Collaborator

sug0 commented Mar 7, 2024

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

@phasewalk1
Copy link
Contributor Author

phasewalk1 commented Mar 9, 2024

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.

@sug0
Copy link
Collaborator

sug0 commented Mar 9, 2024

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

@phasewalk1
Copy link
Contributor Author

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

@sug0
Copy link
Collaborator

sug0 commented Mar 9, 2024

yup

@phasewalk1 phasewalk1 force-pushed the phasewalk1/cli-fallible-conversions branch from dff4b4f to 928bb1e Compare March 9, 2024 09:09
@sug0 sug0 requested a review from tzemanovic March 9, 2024 09:15
@phasewalk1
Copy link
Contributor Author

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

@tzemanovic
Copy link
Member

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 phasewalk1 force-pushed the phasewalk1/cli-fallible-conversions branch from 928bb1e to 265183b Compare March 28, 2024 18:56
@Fraccaman Fraccaman mentioned this pull request Apr 15, 2024
@brentstone
Copy link
Collaborator

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

@phasewalk1
Copy link
Contributor Author

@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
Copy link
Contributor Author

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.

@brentstone brentstone force-pushed the phasewalk1/cli-fallible-conversions branch from 265183b to 7ba4802 Compare May 8, 2024 14:46
brentstone added a commit that referenced this pull request May 8, 2024
* phasewalk1/phasewalk1/cli-fallible-conversions:
  address suggested changes
  changelog: add 2832
  propagate the errors
  refactor CliToSdk trait: introduces "infallible_to_sdk" method
@brentstone brentstone merged commit e8d3e1d into anoma:main May 9, 2024
12 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Converting from CLI to SDK types should be fallible
4 participants