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

Revert "Return errors instead of panic (#3782)" #4061

Closed
wants to merge 1 commit into from

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Apr 5, 2019

This reverts commit 985aae5 (#3782)

/cc @rigelrozanski @MarinX


  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: sdkch add [section] [stanza] [message]

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@alessio
Copy link
Contributor

alessio commented Apr 5, 2019

@MarinX please see #4052

@alexanderbez alexanderbez deleted the bez/revert-3782 branch April 5, 2019 19:21
@MarinX
Copy link
Contributor

MarinX commented Apr 5, 2019

As I was using part of cosmos sdk for my side project, I really liked the project and wanted to contribute.
After seeing the issue about the panic/error I felt comfortable on taking it.
While I was fixing the issue, another sub-issue occurred and then another one and then I fall through the rabbit hole.
This was a big PR now. 😟

I don't know why I hadn't stop and try to think about the stuff I was changing.
@rigelrozanski made a very decent review of error handling/panic which I should thought in the first place before submitting.

I f*cked up and I'm sorry for it. 😞
I'm sorry for wasting your time on reviewing my PR

In the future, I will try to keep my PR short and think twice before submitting.
This rejected PR will be definitely my lesson.

Again, I apologize for any inconvenience it caused.

/cc @alessio @alexanderbez

@alexanderbez
Copy link
Contributor Author

@MarinX absolutely no need to apologize! It was a valiant effort and it exposed areas where the SDK needs improvement from a dev UX point of view. Hardly anything comes perfect the first time around. We really appreciate the effort.

As @rigelrozanski suggested, lets have solid discourse and the best course of action and then you can open a PR if you wish :)

@alessio
Copy link
Contributor

alessio commented Apr 5, 2019

Qui sine peccato est vestrum, primus lapidem mittat

We all make mistakes. I could provide you with an extraordinarily long list of errors that I made myself across many projects. It's key to learn from our errors and move on. I take pride in confessing that I've been making loads of rotten mistakes all along my over-15-year long- career - at times with the sole ultimate objective to learn. I did ultimately learn quite a jolly lot.

Your contributions shall always be welcome and appreciated. So will your mistakes.

@rigelrozanski
Copy link
Contributor

@MarinX really no apologies! You didn't mess up we did, There is no way you could have known these design patterns which we've developed, and it ultimately it's actually the sdk-core team's fault for not having these basic ideas more proliferated within the team so it may have been caught earlier on. I'd encourage you to submit more PR's - myself and I'm sure other team members are willing to put in the time to make sure things get done well and properly and aiding your understanding along the way, please don't let this discourage you :) :) :)

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.

4 participants