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

libgetopts: use Result<Match, _Fail> instead of Result alias for getopts return type #16075

Closed
wants to merge 1 commit into from

Conversation

jxs
Copy link
Contributor

@jxs jxs commented Jul 29, 2014

getopts::getopts doc shows Result as the return type, which can be confusing, due the aliased Result to Result<Match, _Fail>

@lilyball
Copy link
Contributor

What's the motivation for this change?

@jxs
Copy link
Contributor Author

jxs commented Jul 29, 2014

sorry, forgot to add description.
The motivation for that pr was to make the method more explicit.
But I only noticed now, across std doc seems to be both the aliased Result and Result<U, T>
maybe this should be pondered and discussed, to see which makes more sense, and make it uniform across std doc

@lilyball
Copy link
Contributor

We also use a Result typedef in std::fmt. If you feel that using a Result typedef here is confusing because std::result::Result is in the prelude, then you should file an issue about it and try to come up with a general solution.

Offhand, we could perhaps have rustdoc detect when a type shadows something in the prelude and adjust instances of that shadowing type to include the module name, e.g. fmt::Result instead of Result.

Incidentally, if you look at the std::fmt documentation right now you'll actually see Result<(), FormatError> instead of Result. This is a bug, rustdoc is inlining type definitions in some cases. If the bug is fixed it would display Result just like getopts does.

@jxs
Copy link
Contributor Author

jxs commented Jul 29, 2014

I think it's confusing because it doesn't show the T type contained in the Result, (i have now seen that you can click on the result and it will show) I will fill a issue instead regarding this
thanks :)

@jxs jxs mentioned this pull request Jul 30, 2014
@alexcrichton
Copy link
Member

Closing, I think that the naming/convention here is ok, and the real bug was filed under #16096.

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