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

Convert Eithers into Results. #12

Merged
merged 1 commit into from
Jul 9, 2017
Merged

Convert Eithers into Results. #12

merged 1 commit into from
Jul 9, 2017

Conversation

birkenfeld
Copy link
Collaborator

I have only done this where the Left variant was an obvious error
indication. There are a few Eithers left that can either stay
(use the either crate in that case?) or be replaced by domain-specific
enums.

Changes in analysis are unchecked, see #9.

Fixes #3

I have only done this where the Left variant was an obvious error
indication.  There are a few Eithers left that can either stay
(use the either crate in that case?) or be replaced by domain-specific
enums.

Changes in `analysis` are unchecked, see #9.

Fixes #3
@birkenfeld birkenfeld requested a review from tcr July 8, 2017 06:21
Right((v, _)) => Right(v)
}
pub fn execParser_<a: 'static>(parser: P<a>, input: InputStream, pos: Position) -> Result<a, ParseError> {
execParser(parser, input, pos, builtinTypeNames(), newNameSupply()).map(|(v, _)| v)
Copy link
Owner

Choose a reason for hiding this comment

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

Nice.

@@ -31,13 +31,15 @@
#![feature(proc_macro)]
#![feature(slice_patterns, box_syntax, box_patterns, fnbox)]
#![allow(unused_parens)]
// Cut down on number of warnings until we manage it.
#![allow(non_snake_case, non_camel_case_types, unused_imports, unused_variables, dead_code)]
Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good.

@tcr
Copy link
Owner

tcr commented Jul 9, 2017

👏👏👏

Looks straightforward to me. I agree about using the either crate and type-specific enums for the rest. Feel free to give that an attempt, or you can just go ahead and land this.

@tcr tcr mentioned this pull request Jul 9, 2017
@birkenfeld
Copy link
Collaborator Author

Thanks! I'll merge this as-is for now, since the other PR depends on it, and make the remaining Either change after that one.

BTW, do you prefer merge or rebase?

@birkenfeld birkenfeld merged commit a710028 into tcr:master Jul 9, 2017
@birkenfeld birkenfeld deleted the either branch July 9, 2017 04:56
@tcr
Copy link
Owner

tcr commented Jul 10, 2017

@birkenfeld Rebase :) But until we get to a viable alpha release, I'm not a stickler for either.

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.

2 participants