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

[2 of #107] Use native Error #109

Merged
merged 3 commits into from
Dec 19, 2020

Conversation

bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Nov 22, 2020

The Result pattern adds a lot of line noise for the extra try and catch, and is rather inflexible in its return types. This PR removes most of the try/catch blocks, the *Result types, and all references to them.

@goanpeca
Copy link
Member

@bollwyvl I think all the same changes from the original big PR ended up here?

Having changes that mix changes and also split things into files make me very nervous and are harder to review.

So my suggestion for this work. Split things first, or change code first but not both, that is too complex.

I would go for let's first change the file structure without changing anythin else, not a single line of code, just copy pasting from one place to another, that way we are sure things are not breaking in subtle and unexpected ways.

From there we can go PR by PR to change a single thing (like this PR attempted)

Cheers @bollwyvl and thanks again for all the hard work!

@bollwyvl
Copy link
Contributor Author

This PR only offers 0e849fe on top of #108, which does the file split.

@bollwyvl bollwyvl changed the title Use native Error [2 of #107] Use native Error Nov 24, 2020
@goanpeca
Copy link
Member

Duh. Silly me. Thanks for clarifying. :)

@goanpeca
Copy link
Member

goanpeca commented Dec 18, 2020

@bollwyvl maybe do a merge/rebase with master?

Also if you want to do a rebase and squash into a single commit even better!

@bollwyvl
Copy link
Contributor Author

Yep, I'll get on it presently, and fire up the next couple steps.

@bollwyvl
Copy link
Contributor Author

Let's wait to just squash merge, rather than re-write what's in this PR...

@bollwyvl
Copy link
Contributor Author

Looking forward to more PRs with changes like +525 −812, before we're through...

@goanpeca goanpeca merged commit a1428f5 into conda-incubator:master Dec 19, 2020
@goanpeca goanpeca added this to the v2.1.0 milestone Dec 19, 2020
@bollwyvl
Copy link
Contributor Author

bollwyvl commented Dec 20, 2020 via email

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.

None yet

2 participants