-
Notifications
You must be signed in to change notification settings - Fork 991
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
cli: Propagating errors from fallible CliToSdk
conversions
#2832
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
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.
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 |
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
if you run into conflicts, I recommend installing Sublime Merge on macOS/Windows/Linux, or Meld on Windows/Linux to resolve them. cheers, |
@sug0 cool, no conflicts. do i just force push to my remote now? |
yup |
dff4b4f
to
928bb1e
Compare
hey @tzemanovic! I saw these changes got approved, should I rebase again and update my branch? |
pls do just to get a CI run |
928bb1e
to
265183b
Compare
@phasewalk1 Can you rebase this PR on the latest release v0.33? Then also make sure to run |
@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. |
265183b
to
7ba4802
Compare
* phasewalk1/phasewalk1/cli-fallible-conversions: address suggested changes changelog: add 2832 propagate the errors refactor CliToSdk trait: introduces "infallible_to_sdk" method
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