Skip to content

Conversation

nvb
Copy link
Contributor

@nvb nvb commented Jul 20, 2017

This includes a few changes:

  • Use the ? operator in place of the try! macro. This new operator
    stabilized a few versions ago and is now the preferred syntax.
  • Use loop break value functionality, which stabilized in Rust
    1.19
    .
  • Reformat using rustfmt, which seems to have changed since it was last
    run on this code.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@cockroach-teamcity
Copy link
Member

@tamird
Copy link
Contributor

tamird commented Jul 20, 2017

:lgtm:


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


_includes/app/basic-sample.rs, line 7 at r1 (raw file):

fn main() {
    let conn = Connection::connect("postgresql://maxroach@localhost:26257/bank", TlsMode::None)
        .unwrap();

while you're here, want to replace these unwraps with expects?


_includes/app/txn-sample.rs, line 21 at r1 (raw file):

            r => break r,
        }
    };

for bonus points, you can remove res entirely, right?


Comments from Reviewable

This includes a few changes:
- Use the `?` operator in place of the `try!` macro. This new operator
  stabilized a few versions ago and is now the
[preferred](https://doc.rust-lang.org/std/macro.try.html) syntax.
- Use `loop break value` functionality, which stabilized in [Rust
  1.19](https://blog.rust-lang.org/2017/07/20/Rust-1.19.html).
- Reformat using `rustfmt`, which seems to have changed since it was last
  run on this code.
@nvb nvb force-pushed the nvanbenschoten/rust1.19 branch from cc7afc5 to de3fec1 Compare July 21, 2017 03:18
@cockroach-teamcity
Copy link
Member

@nvb
Copy link
Contributor Author

nvb commented Jul 21, 2017

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


_includes/app/basic-sample.rs, line 7 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

while you're here, want to replace these unwraps with expects?

Ehh, I'd rather not clutter the example application with a bunch of .expect("it should connect, duh") lines. This is just for documentation, and I don't think there are any cases in here where a message would help with comprehension.


_includes/app/txn-sample.rs, line 21 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

for bonus points, you can remove res entirely, right?

Done. Good call!


Comments from Reviewable

@tamird
Copy link
Contributor

tamird commented Jul 21, 2017

Reviewed 1 of 1 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@nvb nvb merged commit 4936e42 into master Jul 21, 2017
@nvb nvb deleted the nvanbenschoten/rust1.19 branch July 21, 2017 13:14
@jseldess
Copy link
Contributor

LGTM. Thanks for the update, @nvanbenschoten.

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