Skip to content

raise (Failure ..) -> failwith ..#245

Merged
rgrinberg merged 1 commit into
mirage:masterfrom
rgrinberg:raise_failure_style
Jan 27, 2015
Merged

raise (Failure ..) -> failwith ..#245
rgrinberg merged 1 commit into
mirage:masterfrom
rgrinberg:raise_failure_style

Conversation

@rgrinberg
Copy link
Copy Markdown
Member

simple tweak

better written as failwith x
@rgrinberg
Copy link
Copy Markdown
Member Author

The travis failure is unrelated and should be ignored.

Comment thread async/cohttp_async.ml
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd rather say you are using exceptions for something unexceptional.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes all of this stuff is very much less than ideal. I think we are saving the refactoring post 1.0 however.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why post 1.0? shouldn't such API changes (I suspect raise will be transformed to use an 'a option return type instead) be before 1.0?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well we did ask people to participate in making a checklist for 1.0:

#198

and the issue wasn't included. I guess it's not too late now :P.

EDIT: Net isn't being exported so we can easily fix it now actually.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@hannesm @dbuenzli #247 addresses this

rgrinberg added a commit that referenced this pull request Jan 27, 2015
@rgrinberg rgrinberg merged commit 1b462a4 into mirage:master Jan 27, 2015
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.

3 participants