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

[feature] Make transit_model::Result generic on Error #698

Merged
merged 1 commit into from
Sep 3, 2020

Conversation

woshilapin
Copy link
Contributor

@woshilapin woshilapin commented Sep 3, 2020

This small PR takes inspiration from this little tip.

Note - This is not a breaking change even if we change the type transit_model::Result which is part of our API.

@datanel
Copy link
Member

datanel commented Sep 3, 2020

Is it in Draft for discussion or you are working on it? (label WIP)

@woshilapin
Copy link
Contributor Author

It's a draft for discussion, that's why I didn't put the wip label.

@datanel
Copy link
Member

datanel commented Sep 3, 2020

but we never put wip when it's draft. anyway
It seems good to me

@woshilapin
Copy link
Contributor Author

woshilapin commented Sep 3, 2020

Another side effect is this. Before, the documentation would appear as

image

Now, it will appear as

image

And when the error type is already transit_model::Error, the documentation does not change, it is showing the same thing as before (does not show the error type when it is the default one).

image

StdResult felt like a new type: the reader of the documentation would need to check to understand (arguably one more click to understand the signature of the function). Now, it shows like the standard Result: the reader already knows... although, it's a little bit misleading because it looks like it is the standard Result but in fact, it is transit_model::Result. Personally, I'm fine with this because I don't see any impact of this misleading.

@woshilapin woshilapin marked this pull request as ready for review September 3, 2020 09:03
@woshilapin woshilapin merged commit 9a3d425 into hove-io:master Sep 3, 2020
@woshilapin woshilapin deleted the generic-result branch September 3, 2020 09:03
@woshilapin woshilapin restored the generic-result branch September 3, 2020 09:03
@woshilapin woshilapin deleted the generic-result branch September 3, 2020 09:04
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