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.

Sign up for GitHub

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

Jump to bottom

Substitute unwrap for ? operator #466

Merged
merged 11 commits into from Nov 6, 2020
Merged

Substitute unwrap for ? operator #466

merged 11 commits into from Nov 6, 2020

Conversation

amadeusine
Copy link
Contributor

I'm submitting a refactor (and fixes for test cases).

Description

This PR addresses C-QUESTION-MARK in the roadmap for #191. The examples have been updated to accommodate for the ? operator instead of .unwrap or try!.

Motivation

UUID can move forward with other portions of documentation given this has been completed.

Tests

All existing tests remain successful after changes.

Related Issue(s)

#191

@amadeusine amadeusine marked this pull request as ready for review March 15, 2020 20:07
@Dylan-DPC-zz Dylan-DPC-zz changed the title WIP: Substitute unwrap for ? operator Substitute unwrap for ? operator Mar 15, 2020
Dylan-DPC-zz
Dylan-DPC-zz previously approved these changes Mar 15, 2020
View reviewed changes
@Dylan-DPC-zz
Copy link
Member

bors: r+

bors bot added a commit that referenced this pull request Mar 15, 2020
@bors
Merge #466
651987b 
466: Substitute unwrap for ? operator r=Dylan-DPC a=amadeusine

<!--
    If this PR is a breaking change, ensure that you are opening it against 
    the `breaking` branch.  If the pull request is incomplete, prepend the Title with WIP: 
-->

**I'm submitting a** refactor (and fixes for test cases).

# Description

This PR addresses C-QUESTION-MARK in the roadmap for #191. The examples have been updated to accommodate for the `?` operator instead of `.unwrap` or `try!`.

# Motivation

UUID can move forward with other portions of documentation given this has been completed.

# Tests
<!-- How are these changes tested? -->

All existing tests remain successful after changes.

# Related Issue(s)

#191 


Co-authored-by: Samuel Lim <sclg9w@mail.umkc.edu>
@Dylan-DPC-zz
Copy link
Member

@kinggoesgaming can you check the travis configuration if you have time? thanks

@bors
Copy link
Contributor

bors bot commented Mar 16, 2020

Build failed

@kinggoesgaming
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Aug 12, 2020
@bors
Try #466:
af4c0b2
@bors
Copy link
Contributor

bors bot commented Aug 12, 2020

try

Build failed:

@kinggoesgaming
Copy link
Member

An update: I have been trying to get the one heck of a mess that is our build system fixed. This might take sometime to get fixed. I want a good fix not just a system held together with duct tape.

kinggoesgaming
kinggoesgaming previously approved these changes Sep 5, 2020
View reviewed changes
Copy link
Member

@kinggoesgaming kinggoesgaming left a comment

Choose a reason for hiding this comment

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

Thank you for the PR and sorry for the long wait

kinggoesgaming
kinggoesgaming requested changes Sep 5, 2020
View reviewed changes
Copy link
Member

@kinggoesgaming kinggoesgaming left a comment

Choose a reason for hiding this comment

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

Hey

If you can get the changes made, I will get the PR merged

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@amadeusine
Copy link
Contributor Author

Oops, prematurely pushed here. I'll fix it on my end.

@kinggoesgaming
Copy link
Member

No probs just ping when you are ready

@kinggoesgaming
Copy link
Member

@amadeusine any update?

@amadeusine
Copy link
Contributor Author

amadeusine commented Sep 20, 2020 •
edited

Update on the changes in question: It looks like the v4 example will not pass tests under --all-features as
let my_uuid = Uuid::new_v4()?; propagates getrandom::Error (which doesn't impl std::error::Error unless enabled). While it is possible to explicitly specify fn main() -> Result<(), getrandom::Error>, doing so would then fail on tests with --no-default-features due to getrandom being optional.

If either leaving it as-is (with the recommended changes) or specifying getrandom::Error is acceptable, I can go ahead and push. Suggestions welcome.

@kinggoesgaming
Copy link
Member

I think the best course of action would be to change to ignore,rust to ignore the test for now. The error type is expected to have changes and I think we can cover it under it.

@amadeusine
Copy link
Contributor Author

It looks like the last change to the ignored example was already merged. Would it be fine to close the PR, then?

@KodrAus
Copy link
Member

KodrAus commented Nov 6, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 6, 2020

Build succeeded:

@bors bors bot merged commit 379e3e4 into uuid-rs:master Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kinggoesgaming kinggoesgaming kinggoesgaming approved these changes

@KodrAus KodrAus Awaiting requested review from KodrAus

@Dylan-DPC-zz Dylan-DPC-zz Awaiting requested review from Dylan-DPC-zz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@amadeusine @Dylan-DPC-zz @kinggoesgaming @KodrAus