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

Hide implementation details #271

Merged
merged 6 commits into from
Dec 13, 2015
Merged

Hide implementation details #271

merged 6 commits into from
Dec 13, 2015

Conversation

ixti
Copy link
Member

@ixti ixti commented Dec 12, 2015

This is renewed #261

@ixti
Copy link
Member Author

ixti commented Dec 12, 2015

@zanker @tarcieri personally I have no strong feelings one way or the other, so it's up to you guys if we want this PR to be merged in or not. This is a breaking changes so if we want this to be in - we should do this like now.

@sferik
Copy link
Contributor

sferik commented Dec 12, 2015

👍

@zanker
Copy link
Contributor

zanker commented Dec 12, 2015

I'm a fan, but I'm concerned about breaking changes. Can we leave this for a 2.0?

Sent from my iPhone

On Dec 12, 2015, at 07:12, Erik Michaels-Ober notifications@github.com wrote:


Reply to this email directly or view it on GitHub.

@sferik
Copy link
Contributor

sferik commented Dec 12, 2015

I'm a fan, but I'm concerned about breaking changes. Can we leave this for a 2.0?

IMHO, if we all agree this is better, we should make this change now, before 1.0 is released, rather than kicking the can down the road and causing breakages in the future when (presumably, if everything goes according to plan) this library has even more users.

@zanker
Copy link
Contributor

zanker commented Dec 12, 2015

Whoops, I forgot we only had a pre not an actual 1.0. I'm good with making the change now then.

@tarcieri
Copy link
Member

This looks good to me, although requires a bit of #cause spelunking now if you want to figure out where the original exception occurred (well, I guess we had some rethrows before with different classes, but...)

@zanker and yep, we're just at 1.0.0.pre1 right now, so it's last call for any breaking changes before 1.0

tarcieri added a commit that referenced this pull request Dec 13, 2015
@tarcieri tarcieri merged commit 3588e7a into master Dec 13, 2015
@tarcieri tarcieri deleted the hide_implementation_details branch December 13, 2015 01:55
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.

5 participants