Github user anatol commented on a diff in the pull request:

    https://github.com/apache/thrift/pull/1147#discussion_r97210872
  
    --- Diff: lib/rs/src/errors.rs ---
    @@ -0,0 +1,678 @@
    +// Licensed to the Apache Software Foundation (ASF) under one
    +// or more contributor license agreements. See the NOTICE file
    +// distributed with this work for additional information
    +// regarding copyright ownership. The ASF licenses this file
    +// to you under the Apache License, Version 2.0 (the
    +// "License"); you may not use this file except in compliance
    +// with the License. You may obtain a copy of the License at
    +//
    +//   http://www.apache.org/licenses/LICENSE-2.0
    +//
    +// Unless required by applicable law or agreed to in writing,
    +// software distributed under the License is distributed on an
    +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    +// KIND, either express or implied. See the License for the
    +// specific language governing permissions and limitations
    +// under the License.
    +
    +use std::convert::{From, Into};
    +use std::error::Error as StdError;
    +use std::fmt::{Debug, Display, Formatter};
    +use std::{error, fmt, io, string};
    +use try_from::TryFrom;
    +
    +use ::protocol::{TFieldIdentifier, TInputProtocol, TOutputProtocol, 
TStructIdentifier, TType};
    +
    +// FIXME: should all my error structs impl error::Error as well?
    +// FIXME: should all fields in TransportError, ProtocolError and 
ApplicationError be optional?
    +
    +/// Thrift error type.
    +///
    +/// The error type defined here is used throughout this crate as well as in
    +/// all Thrift-compiler-generated Rust code. It falls into one of four
    +/// standard (i.e. defined by convention in Thrift implementations) 
categories.
    +/// These are:
    +///
    +/// 1. **Transport errors**: errors encountered while writing byes to the 
I/O
    +///    channel. These include "connection closed", "bind errors", etc.
    +/// 2. **Protocol errors**: errors encountered when processing a Thrift 
message
    +///    within the thrift library. These include "exceeding size limits",
    +///    "unsupported protocol versions", etc.
    +/// 3. **Application errors**: errors encountered when 
Thrift-compiler-generated
    +///    'application' code encounters conditions that violate the spec. 
These
    +///    include "out-of-order messages", "missing required-field values", 
etc.
    +///    This category also functions as a catch-all: any error returned by
    +///    handler functions is automatically encoded as an `ApplicationError` 
and
    +///    returned to the caller.
    +/// 4. **User errors**: exception structs defined within the Thrift IDL.
    +///
    +/// With the exception of `Error::User`, each error variant encapsulates a
    +/// corresponding struct which encodes two required fields:
    +///
    +/// 1. kind: an enum indicating what kind of error was encountered
    +/// 2. message: string with human-readable error information
    +///
    +/// These two fields are encoded and sent over the wire to the remote. 
`kind`
    +/// is defined by convention while `message` is freeform. If none of the
    +/// enumerated error kinds seem suitable use the catch-all `Unknown`.
    +///
    +/// # Examples
    +///
    +/// Creating a `TransportError` explicitly.
    +///
    +/// Conversions are defined from `thrift::TransportError`, 
`thrift::ProtocolError`
    +/// and `thrift::ApplicationError` to the corresponding `thrift::Error` 
variant
    +/// (see `err2` below).
    +///
    +/// ```
    +/// use thrift;
    +/// use thrift::{TransportError, TransportErrorKind};
    +///
    +/// let err0: thrift::Result<()> = Err(
    +///   thrift::Error::Transport(
    +///     TransportError {
    +///       kind: TransportErrorKind::TimedOut,
    +///       message: format!("connection to server timed out")
    +///     }
    +///   )
    +/// );
    +///
    +/// let err1: thrift::Result<()> = Err(
    +///   thrift::Error::Transport(
    +///     TransportError::new(
    +///       TransportErrorKind::TimedOut,
    +///       format!("connection to server timed out")
    +///     )
    +///   )
    +/// );
    +///
    +/// let err2: thrift::Result<()> = Err(
    +///   thrift::Error::from(
    +///     TransportError::new(
    +///       TransportErrorKind::TimedOut,
    +///       format!("connection to server timed out")
    +///     )
    +///   )
    +/// );
    +///
    +/// ```
    +///
    +/// Creating an arbitrary error from a string
    +///
    +/// ```
    +/// use thrift;
    +/// use thrift::{ApplicationError, ApplicationErrorKind};
    +///
    +/// let err0: thrift::Result<()> = Err(
    +///   thrift::Error::from("This is an error")
    +/// );
    +///
    +/// // err0 is equivalent to...
    +///
    +/// let err1: thrift::Result<()> = Err(
    +///   thrift::Error::Application(
    +///     ApplicationError::new(
    +///       ApplicationErrorKind::Unknown,
    +///       format!("This is an error")
    +///     )
    +///   )
    +/// );
    +/// ```
    +///
    +/// Returning a user-defined (using the Thrift IDL) exception struct.
    +///
    +/// ```text
    +/// // Thrift IDL exception definition.
    +/// exception Xception {
    +///   1: i32 errorCode,
    +///   2: string message
    +/// }
    +/// ```
    +///
    +/// ```
    +/// use std::convert::From;
    +/// use std::error::Error;
    +/// use std::fmt;
    +/// use std::fmt::{Display, Formatter};
    +///
    +/// // auto-generated by the Thrift compiler
    +/// #[derive(Clone, Debug, Eq, Ord, PartialEq, PartialOrd)]
    +/// pub struct Xception {
    +///   pub error_code: Option<i32>,
    +///   pub message: Option<String>,
    +/// }
    +///
    +/// // auto-generated by the Thrift compiler
    +/// impl Error for Xception {
    +///   fn description(&self) -> &str {
    +///     "remote service threw Xception"
    +///   }
    +/// }
    +///
    +/// // auto-generated by the Thrift compiler
    +/// impl From<Xception> for thrift::Error {
    +///   fn from(e: Xception) -> Self {
    +///     thrift::Error::User(Box::new(e))
    +///   }
    +/// }
    +///
    +/// // auto-generated by the Thrift compiler
    +/// impl Display for Xception {
    +///   fn fmt(&self, f: &mut Formatter) -> fmt::Result {
    +///     self.description().fmt(f)
    +///   }
    +/// }
    +///
    +/// // in user code...
    +/// let err: thrift::Result<()> = Err(
    +///   thrift::Error::from(Xception { error_code: Some(1), message: None })
    +/// );
    +/// ```
    +pub enum Error {
    +    /// Errors encountered while writing byes to the I/O channel.
    +    ///
    +    /// These include "connection closed", "bind errors", etc.
    +    Transport(TransportError),
    +    /// Errors encountered when processing a Thrift message within the rift
    +    /// library.
    +    ///
    +    /// These include "exceeding size limits", "unsupported protocol 
versions",
    +    /// etc.
    +    Protocol(ProtocolError),
    +    /// Errors encountered when Thrift-compiler-generated 'application' 
code
    +    /// encounters conditions that violate the spec.
    +    ///
    +    /// These include "out-of-order messages", "missing required-field 
values",
    +    /// etc. This variant also functions as a catch-all: any error 
returned by
    +    /// handler functions is automatically encoded as an 
`ApplicationError` and
    +    /// returned to the caller.
    +    Application(ApplicationError),
    +    /// Carries exception structs defined within the Thrift IDL.
    +    User(Box<error::Error + Sync + Send>),
    +}
    +
    +impl Error {
    +    /// Read the wire representation of an application exception thrown by 
the
    +    /// remote Thrift endpoint into its corresponding rift 
`ApplicationError`
    +    /// representation.
    +    ///
    +    /// Application code should never call this method directly.
    +    pub fn read_application_error_from_in_protocol(i: &mut TInputProtocol)
    +                                                   -> 
::Result<ApplicationError> {
    +        let mut message = "general remote error".to_owned();
    +        let mut kind = ApplicationErrorKind::Unknown;
    +
    +        try!(i.read_struct_begin());
    --- End diff --
    
    It should probably use shorter and nicer `?` operator instead of `try!` 
macro. Here and everywhere else. I counted ~50 usages of `try!` macro.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to