You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by allengeorge <gi...@git.apache.org> on 2017/01/01 18:13:50 UTC
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
GitHub user allengeorge opened a pull request:
https://github.com/apache/thrift/pull/1147
THRIFT-2945 Add Rust support
This is a PR to add Rust support to Thrift. It is based on today's master, and I've verified that Rust server/client successfully communicates with all cross-platform clients and servers (*). I would be happy to accept and incorporate feedback!
Not implemented:
* Struct/union constants: Honestly, this looks like it's "not possible" (tm)
* Multiplexed processor
I will be continuing to add documentation, comments and clean up the code in both the C++ generator as well as the Rust client library.
(*) With exception of:
1. D: no D compiler installed
2. Lua: need to figure out a way to use `.dylib` when linking against Lua libs
3. Dart: weird problem with pubspec requiring 1.0.0 instead of 1.0.0-dev
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/allengeorge/thrift thrift-2945-pr
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1147.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1147
----
commit 79128c3fab9cce7d0040a1528f122c2d982f8c7d
Author: Allen George <al...@gmail.com>
Date: 2016-11-02T12:01:08Z
THRIFT-2945 Add Rust support
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
I've now updated the PR with `TMultiplexedProcessor`, which was the last functional piece missing. I'm now fleshing out the remaining tests as well as documentation and examples.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
@jsgf For structs it's more involved than it appears - perhaps because of the way I've chosen to represent them. Here are some factors that cause issues:
1. I represent `required` fields as bare types (i.e. no `Option` wrapper). This means that if the user doesn't specify them in the IDL const declaration then `rustc` will fail.
2. I'm not sure how to handle a case where a user specifies a single struct field in the IDL const declaration but ignores the others. I *could* conceivably use ".." but (see below)
3. I currently don't implement a `Defaults` trait for structs. Even if I did generate one, I'd only do it for structs with all optional or opt-in-req-out fields
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by anatol <gi...@git.apache.org>.
Github user anatol commented on the issue:
https://github.com/apache/thrift/pull/1147
@allengeorge as of your question about `thrift` crate. Someone from Thrift team should own access to it and deploy a new library version every time Thrift makes a release.
For example [thrift rubygem](https://rubygems.org/gems/thrift/) is owned by @jfarrell. @jfarrell maybe you have a suggestion who from Thrift team needs to make Rust library releases? Or maybe [crate.io](crate.io) can be co-owned with @allengeorge who will deploy new Rust library on behalf of Thrift team?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94346650
--- Diff: test/ThriftTest.thrift ---
@@ -67,7 +67,7 @@ typedef i64 UserId
struct Bonk
{
1: string message,
- 2: i32 type
+ 2: i32 bonkType
}
--- End diff --
Well, it is a keyword in Delphi too. How did we do that? Magic? No.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
OK. I believe I've addressed all outstanding comments.
@anatol:
* Reserved type names now have a `_` suffix. I've confirmed that this does not trip warnings for either snake-cased or camel-cased identifiers
* Ran `rustfmt` with default settings on the code
* Rebased onto master and squashed all the changes into a single commit
@Jens-G:
* Should the version of the client library be 1.0.0?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94510188
--- Diff: compiler/cpp/src/thrift/generate/t_rs_generator.cc ---
@@ -0,0 +1,2846 @@
+/*
+ * 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.
+ */
+
+#include <string>
+#include <fstream>
+#include <iostream>
+#include <boost/algorithm/string/replace.hpp>
--- End diff --
Oops! The Thrift compiler should not rely on boost being installed. We really need to remove that. Sorry.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94303703
--- Diff: lib/rs/Cargo.toml ---
@@ -0,0 +1,17 @@
+[package]
+name = "rift"
+description = "Rust Thrift library"
+version = "0.5.1"
+license = "Apache-2.0"
+authors = ["Allen George <al...@gmail.com>"]
+readme = "README.md"
+documentation = "https://docs.rs/rift/"
+exclude = ["Makefile*"]
+keywords = ["thrift"]
+
--- End diff --
That should be adjusted too.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
@Jens-G Could you add `lib/rs/README.md` to the list of files for which a version has to be updated? I will also look and see if I can automate this somehow.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
I've now added the tutorial.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94354857
--- Diff: test/ThriftTest.thrift ---
@@ -67,7 +67,7 @@ typedef i64 UserId
struct Bonk
{
1: string message,
- 2: i32 type
+ 2: i32 bonkType
}
--- End diff --
@Jens-G I'm sorry if my comment came across as rude. I'll change the code generator to prefix reserved names with an identifier.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94331438
--- Diff: lib/rs/Cargo.toml ---
@@ -0,0 +1,17 @@
+[package]
+name = "rift"
+description = "Rust Thrift library"
+version = "0.5.1"
+license = "Apache-2.0"
+authors = ["Allen George <al...@gmail.com>"]
+readme = "README.md"
+documentation = "https://docs.rs/rift/"
+exclude = ["Makefile*"]
+keywords = ["thrift"]
+
--- End diff --
What should change? And what should I change it to?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94297395
--- Diff: test/csharp/TestServer.cs ---
@@ -273,39 +273,24 @@ public long testTypedef(long thing)
return mapmap;
}
+ // Insanity
+ // returns:
+ // { 1 => { 2 => argument,
+ // 3 => argument,
+ // },
+ // 2 => { 6 => <empty Insanity struct>, },
+ // }
public Dictionary<long, Dictionary<Numberz, Insanity>> testInsanity(Insanity argument)
{
testLogDelegate.Invoke("testInsanity()");
- Xtruct hello = new Xtruct();
--- End diff --
I fixed exactly that just a few days ago for Delphi and dotnetcore. I'm going to make a separate ticket for it, don't worry.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
I'm continuing to add tests and update documentation, and I think I've addressed all the initial comments on this PR. Any further thoughts?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
Yay! Build passed :) Any thoughts on whether this is OK to merge?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
@anatol Ran `rustfmt` with default settings. Humorously, it has a bug where it transforms `::{SomeTypeName}` to `::::{SomeTypeName}`.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
@anatol My apologies for the earlier spam. FWIW, the latest version of the PR has the following:
- The "thrift" crate name (I do not use "rift" anywhere)
- I use the "_" suffix instead of the prefixes I used earlier
I am just about to submit another change that removes the ".erl" gitignores.
I would be very happy to be the thrift library maintainer for rust. There are additional server types I want to add, including one for Tokio when it stabilizes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r96126529
--- Diff: lib/rs/Cargo.toml ---
@@ -0,0 +1,17 @@
+[package]
+name = "rift"
--- End diff --
@anatol I've previously changed this - perhaps you accidentally looked at an older version of the PR?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94331401
--- Diff: test/ThriftTest.thrift ---
@@ -67,7 +67,7 @@ typedef i64 UserId
struct Bonk
{
1: string message,
- 2: i32 type
+ 2: i32 bonkType
}
--- End diff --
@Jens-G will do, but now the autogenerated rust code won't work :(
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94371350
--- Diff: test/ThriftTest.thrift ---
@@ -67,7 +67,7 @@ typedef i64 UserId
struct Bonk
{
1: string message,
- 2: i32 type
+ 2: i32 bonkType
}
--- End diff --
It didn't.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by markerickson-wf <gi...@git.apache.org>.
Github user markerickson-wf commented on the issue:
https://github.com/apache/thrift/pull/1147
@allengeorge I just did `make clean` followed by `make precross` and I don't see any warnings.
The `pubspec.lock` files are in `.gitignore`, so you can safely delete those and try again.
`make clean` should remove all of the artifacts leftover from `pub get`, but if you want to make sure, you could do this from the root of the thrift repo:
```
find . -type f -name "pubspec.lock" | xargs rm
find . -type d -name ".pub" | xargs rm -r
find . -type d -name "packages" | xargs rm -r
find . -type f -name ".packages" | xargs rm
```
As far as the `version` in `pubspec.yaml`, that should be updated on the next release. It looks like it's on the checklist here - https://thrift.apache.org/docs/committers/HowToVersion
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
With my latest commits (fix type reference that broke on another version of clang) and removing boost, it appears like the code compiles on all platforms. It's not clear to me why the debian builds are breaking though. Did I forget something?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by anatol <gi...@git.apache.org>.
Github user anatol commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94333540
--- Diff: test/ThriftTest.thrift ---
@@ -67,7 +67,7 @@ typedef i64 UserId
struct Bonk
{
1: string message,
- 2: i32 type
+ 2: i32 bonkType
}
--- End diff --
@Jens-G "type" is a Rust keyword https://doc.rust-lang.org/grammar.html#keywords What is the best way to deal in such situation?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r97211074
--- 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 --
Very well. I'll change it. Please note that I started this in an older version of Rust (1.12) that didn't have the ? operator, and by the time 1.13 came out I'd over a thousand lines of code already written.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by anatol <gi...@git.apache.org>.
Github user anatol commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r96123141
--- Diff: .gitignore ---
@@ -281,6 +293,7 @@ project.lock.json
/test/log/
/test/test.log
/test/erl/.generated
+/test/erl/.rebar
--- End diff --
ditto
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
OK. Will do.
@anatol Do you know if it's possible for someone on the thrift team to "reserve" the thrift crate on crates.io? I originally named the library `rift` because I didn't want to claim the name until it was decided that this code was reasonable.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
More updates:
- Minor cleanups to Rust code generators
- Confirmed that all Rust code does not trigger any clippy lint warnings
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by anatol <gi...@git.apache.org>.
Github user anatol commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r97211272
--- 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 --
Yes I understand that. Rust is very actively developed language.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r97211560
--- 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 --
@anatol Yup. That said - I'll fix up the usage of `try!` and replace it with `?`
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
Updated:
- Auto-generate constructors for all public structs
- Auto-generate `Default` impl for structs with all-optional fields
- Add constructors for protocol/transport factories
- Use constructors/defaults where appropriate in test/tutorial, etc.
- Fix `make clean` bugs in Rust makefiles
- Clean up some generator code
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94370827
--- Diff: lib/rs/Cargo.toml ---
@@ -0,0 +1,17 @@
+[package]
+name = "rift"
+description = "Rust Thrift library"
+version = "0.5.1"
+license = "Apache-2.0"
+authors = ["Allen George <al...@gmail.com>"]
+readme = "README.md"
+documentation = "https://docs.rs/rift/"
+exclude = ["Makefile*"]
+keywords = ["thrift"]
+
--- End diff --
Yep, fine. We also have a template on the bottom of [that page over here]((http://thrift.apache.org/docs/committers/HowToVersion)). The `name` should probably not be "Rift" and `documentation` should point to http://thrift.apache.org. The `version` tag needs to be the same as for all other client libraries.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
@anatol Code has been updated with the following changes:
- All usage of `try!` converted to `?`
- Fix all but two clippy lint warnings (I thought the existing code was clearer)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
Sorry @anatol - I thought I'd changed the package name everywhere! I must have missed that. Let me fix it ASAP. Sorry!
On Sat, Jan 14, 2017 at 3:20 PM -0500, "Anatol Pomozov" <no...@github.com> wrote:
This patch looks really good. Thank you @allengeorge for your work! Thrift team, is there anything left that prevents rust library from merging?
Is there a such status as 'thrift library maintainer'. It would be great if @allengeorge keep maintaining rust library and thrift cargo.
\u2014
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by anatol <gi...@git.apache.org>.
Github user anatol commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94364636
--- Diff: lib/rs/Cargo.toml ---
@@ -0,0 +1,17 @@
+[package]
+name = "rift"
--- End diff --
I still think that `thrift` would be a better name for the official Thrift library implementation.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
@anatol Thanks for the clippy output! I'll look through all of the items and fix them. I'd no idea that `new()` without `Default` was considered a code smell. Interesting.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by anatol <gi...@git.apache.org>.
Github user anatol commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r96123137
--- Diff: .gitignore ---
@@ -206,6 +206,7 @@ project.lock.json
/lib/delphi/test/typeregistry/*.local
/lib/delphi/test/typeregistry/*.dcu
/lib/erl/.generated
+/lib/erl/.rebar
--- End diff --
it seems unrelated to the rust library
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r96126542
--- Diff: .gitignore ---
@@ -281,6 +293,7 @@ project.lock.json
/test/log/
/test/test.log
/test/erl/.generated
+/test/erl/.rebar
--- End diff --
Will do so.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1147
Master is (currently) always at 1.0.0, since we haven't released a 1.0 yet.
If you tell me all the relevant files, I can add it to
https://thrift.apache.org/docs/committers/HowToVersion
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1147
> Oh. Interesting. I forgot that Debian doesn't have a Rust package (yet) though it's being worked on IIRC.
There is no (suitable) Rust package for Suse either, only an older version. I finally managed [installing through the Rust web site](https://www.rust-lang.org/en-US/install.html), works like a charm & build went through.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
@Jens-G Do you know why the Debian steps in Travis are failing with:
```
The command "docker run --net=host -e BUILD_LIBS="$BUILD_LIBS" $BUILD_ENV -v $(pwd):/thrift/src -it thrift-build:$DISTRO build/docker/scripts/$SCRIPT $BUILD_ARG" exited with 2.```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94287260
--- Diff: test/ThriftTest.thrift ---
@@ -115,14 +115,6 @@ struct CrazyNesting {
4: binary binary_field
}
-union SomeUnion {
--- End diff --
I, uhh...am not sure why this got removed. I'll re-add it. Suffice it to say, the code handles unions.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
Any thoughts on this?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94347166
--- Diff: test/ThriftTest.thrift ---
@@ -67,7 +67,7 @@ typedef i64 UserId
struct Bonk
{
1: string message,
- 2: i32 type
+ 2: i32 bonkType
}
--- End diff --
BTW, I just did a search for *.thrift files already using `type` across the Thrift repo. Besides ThriftTest.thrift itself, there are quite a few more. So if we put "type" into the global list, we will have to fix a whole lot of existing code.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1147
@all: Everyone happy with it?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
Also, I think the Appveyor build breakage is unrelated?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r96126537
--- Diff: .gitignore ---
@@ -206,6 +206,7 @@ project.lock.json
/lib/delphi/test/typeregistry/*.local
/lib/delphi/test/typeregistry/*.dcu
/lib/erl/.generated
+/lib/erl/.rebar
--- End diff --
I'll remove this.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
@markerickson-wf Thanks Mark. Here's the error I get when I run `make precross`:
```
Resolving dependencies...
The pubspec for thrift 0.10.0 from path has version 1.0.0-dev.
make[4]: *** [pub-get-client] Error 65
```
I see the `pubspec.yaml` in `lib/dart` is set to `1.0.0-dev`, but what's weird (to me) is that the `pubspec.yaml` in the `test/dart` directory only has a path dependency, not a version dependency. There must be a lock file with the specific version somewhere that I missed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
@jsgf I opted not to do anything with Tokio because I'd like to see it settle down and get some solid real-world usage for a while before coding anything up with it. IMO, it would be great to get Rust into the hands of people (like me) who'd be OK with writing Rust clients against non-Rust servers.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
What should the version number of the client library be? Looks like they're currently set to 1.0.0?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by anatol <gi...@git.apache.org>.
Github user anatol commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r97210978
--- 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 --
The [doc](https://doc.rust-lang.org/std/macro.try.html) says
```
Prefer using ? syntax to try!. ? is built in to the language and is more succinct than try!. It is the standard method for error propagation.
```
So `?` is more Rust-idiomatic and future-proof way to check the result.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94576295
--- Diff: compiler/cpp/src/thrift/generate/t_rs_generator.cc ---
@@ -0,0 +1,2846 @@
+/*
+ * 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.
+ */
+
+#include <string>
+#include <fstream>
+#include <iostream>
+#include <boost/algorithm/string/replace.hpp>
--- End diff --
OK. I'll code up my own string replace.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
Fixed makefile errors with `make distdir`
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94302235
--- Diff: test/ThriftTest.thrift ---
@@ -67,7 +67,7 @@ typedef i64 UserId
struct Bonk
{
1: string message,
- 2: i32 type
+ 2: i32 bonkType
}
--- End diff --
Please revert that.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94356759
--- Diff: lib/rs/Cargo.toml ---
@@ -0,0 +1,17 @@
+[package]
+name = "rift"
+description = "Rust Thrift library"
+version = "0.5.1"
+license = "Apache-2.0"
+authors = ["Allen George <al...@gmail.com>"]
+readme = "README.md"
+documentation = "https://docs.rs/rift/"
+exclude = ["Makefile*"]
+keywords = ["thrift"]
+
--- End diff --
I have changed it to "Apache Thrift Developers <de...@thrift.apache.org>". This is what other author tags read, so I assume that's OK.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94287266
--- Diff: test/csharp/TestServer.cs ---
@@ -273,39 +273,24 @@ public long testTypedef(long thing)
return mapmap;
}
+ // Insanity
+ // returns:
+ // { 1 => { 2 => argument,
+ // 3 => argument,
+ // },
+ // 2 => { 6 => <empty Insanity struct>, },
+ // }
public Dictionary<long, Dictionary<Numberz, Insanity>> testInsanity(Insanity argument)
{
testLogDelegate.Invoke("testInsanity()");
- Xtruct hello = new Xtruct();
--- End diff --
I had to fix the C# server because it was not handling `testInsanity` properly.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
I thought that there was no agreed-upon rust codestyle... At least, I think
rustfmt's defaults are winding their way through an RFC process right now
no?
Terminal Musings: http://www.allengeorge.com/
Raft in Java: https://github.com/allengeorge/libraft/
Twitter: https://twitter.com/allenageorge/
On Sun, Jan 1, 2017 at 10:03 PM, Anatol Pomozov <no...@github.com>
wrote:
>
> - Non-Rust changes (like C# test fix) should be extracted into
> separate change.
> - Could you please run 'rustfmt' tool over rust sources to bring the
> sources to common codestyle?
>
> \u2014
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/thrift/pull/1147#issuecomment-269931189>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AAEO9Ip_tSvLC40JKplteG7lE2YGAowbks5rOGjwgaJpZM4LYwAn>
> .
>
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
[puzzled] I removed the reserved prefix and added an underscore suffix as you requested. Perhaps I didn't commit that. Let me check. Sorry about that!
And, BTW - I would be very happy to maintain the Rust thrift library and cargo. I have additional changes I'd like to make, including a Tokio implementation.
On Sat, Jan 14, 2017 at 3:24 PM -0500, "Allen George" <al...@gmail.com> wrote:
Sorry @anatol - I thought I'd changed the package name everywhere! I must have missed that. Let me fix it ASAP. Sorry!
On Sat, Jan 14, 2017 at 3:20 PM -0500, "Anatol Pomozov" <no...@github.com> wrote:
This patch looks really good. Thank you @allengeorge for your work! Thrift team, is there anything left that prevents rust library from merging?
Is there a such status as 'thrift library maintainer'. It would be great if @allengeorge keep maintaining rust library and thrift cargo.
\u2014
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r97210903
--- 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 --
That was a conscious choice. Personally, I find the try! Macro much easier to read in my IDE
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
As for crate ownership, I'm happy to publish (and thereby reserve `thrift`), and then transfer ownership to whoever will be in charge of pushing published crates.
The relevant documentation is here: http://doc.crates.io/crates-io.html#cargo-owner
Note: I've not done anything yet.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
Oh. Interesting. I forgot that Debian doesn't have a Rust package (yet) though it's being worked on IIRC.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by anatol <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
@anatol I've made the changes. I'll try and rebase (it's a little tough because I merged from master in the middle of my work to pick up some build fixes)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94331337
--- Diff: test/csharp/TestServer.cs ---
@@ -273,39 +273,24 @@ public long testTypedef(long thing)
return mapmap;
}
+ // Insanity
+ // returns:
+ // { 1 => { 2 => argument,
+ // 3 => argument,
+ // },
+ // 2 => { 6 => <empty Insanity struct>, },
+ // }
public Dictionary<long, Dictionary<Numberz, Insanity>> testInsanity(Insanity argument)
{
testLogDelegate.Invoke("testInsanity()");
- Xtruct hello = new Xtruct();
--- End diff --
Thank you @Jens-G
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by anatol <gi...@git.apache.org>.
Github user anatol commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94457731
--- Diff: compiler/cpp/src/thrift/generate/t_rs_generator.cc ---
@@ -2722,7 +2756,30 @@ string t_rs_generator::rust_namespace(t_service* tservice) {
}
string t_rs_generator::rust_struct_name(t_struct* tstruct) {
- return rust_camel_case(tstruct->get_name());
+ string base_struct_name(rust_camel_case(tstruct->get_name()));
+ if (is_reserved(base_struct_name)) {
+ return "Res" + base_struct_name;
+ } else {
+ return base_struct_name;
+ }
+}
+
+string t_rs_generator::rust_field_name(t_field* tfield) {
+ string base_field_name(rust_snake_case(tfield->get_name()));
+ if (is_reserved(base_field_name)) {
+ return "res_" + base_field_name;
--- End diff --
"res_" prefix some might be a little bit confusing. Maybe just add "_" prefix/suffix to the name? Compare field names: `res_type` and `type_`; `res_func` and `func_`.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
@anatol No worries! I was glad to do it :)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r96126534
--- Diff: compiler/cpp/src/thrift/generate/t_rs_generator.cc ---
@@ -2722,7 +2756,30 @@ string t_rs_generator::rust_namespace(t_service* tservice) {
}
string t_rs_generator::rust_struct_name(t_struct* tstruct) {
- return rust_camel_case(tstruct->get_name());
+ string base_struct_name(rust_camel_case(tstruct->get_name()));
+ if (is_reserved(base_struct_name)) {
+ return "Res" + base_struct_name;
+ } else {
+ return base_struct_name;
+ }
+}
+
+string t_rs_generator::rust_field_name(t_field* tfield) {
+ string base_field_name(rust_snake_case(tfield->get_name()));
+ if (is_reserved(base_field_name)) {
+ return "res_" + base_field_name;
--- End diff --
I agree. I've changed this as well - perhaps you were looking at an older version of the PR?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94346945
--- Diff: test/ThriftTest.thrift ---
@@ -67,7 +67,7 @@ typedef i64 UserId
struct Bonk
{
1: string message,
- 2: i32 type
+ 2: i32 bonkType
}
--- End diff --
Well, first, it is a keyword in Delphi too. Next, you cannot safely assume that anyone offering a Thrift API will test all languages implemented today if they work with the curent names you use in your IDL. And you cannot look into the future and foresee what language Apple or Google or whoever else comes around next week which may use "bonkType" as a keyword. So we have to deal with it. One way to do it is like the C# generator does it (look for `csharp_keywords`). The only bad thing about it is that we still have no common mechanism that is used by all generators. Today all generators more or less implement it their own way. I still have it on my list and some day I'll start working on it and finally consolidate that stuff.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by markerickson-wf <gi...@git.apache.org>.
Github user markerickson-wf commented on the issue:
https://github.com/apache/thrift/pull/1147
@allengeorge re: "Dart: weird problem with pubspec requiring 1.0.0 instead of 1.0.0-dev" - I can help look into that if you give some more detail.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by jsgf <gi...@git.apache.org>.
Github user jsgf commented on the issue:
https://github.com/apache/thrift/pull/1147
Awesome! The thing that I got stalled on was non-support for other protocols/framing, which meant it wasn't really useful for real use. The other blocker was support for async, which is pretty much a must-have for practical uses, and the Rust async story is in flux right now with Tokio, etc.
> Not implemented:
> - Struct/union constants: Honestly, this looks like it's "not possible" (tm)
> - Multiplexed processor
Have a look at [my repo](https://github.com/jsgf/thrift/tree/rust). I implemented these with `lazy_static`. I didn't find anything that couldn't be implemented; in general Rust and Thrift seem like a good match.
The main complexity I found was that you have to use `BTreeMap`/`BTreeSet` if you want to use maps/sets as keys for other maps/sets, but otherwise `HashMap`/`HashSet` are preferred.
The other tricky part was using `Result` to represent exceptions and handling some edge cases around void functions which are extended to throw extensions.
I'll review in detail.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
@Jens-G Sorry to bother you, but all the Appveyor builds seem stalled. It appears to be holding up the Travis fix #1158 that was approved yesterday as well :/
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by anatol <gi...@git.apache.org>.
Github user anatol commented on the issue:
https://github.com/apache/thrift/pull/1147
@allengeorge thanks for this great work!! A few comment I had earlier that looks unresolved
- Name of the official crate should be `thrift`, not `rift`.
- Fields with reserved names should be renamed to `type_` rather than to `res_type`.
Also please squash all commits in `thrift-2945-pr` and rebase if needed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
The only file that has to be changed is `lib/rs/Cargo.toml`. It's a simple INI-style file, and it has a `version` field in there. Note that Rust does not allow you to overwrite crates, so if someone accidentally releases a 1.0.0 onto https://crates.io the version is going to have to be bumped upwards.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
Thanks @markerickson-wf - I also saw the commit improving "make clean".
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by anatol <gi...@git.apache.org>.
Github user anatol commented on the issue:
https://github.com/apache/thrift/pull/1147
FYI: run the code through `clippy` checker and [here is its output](http://pastebin.com/icvD3yNA) Some of the warnings (but not all) look interesting.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94347222
--- Diff: lib/rs/Cargo.toml ---
@@ -0,0 +1,17 @@
+[package]
+name = "rift"
+description = "Rust Thrift library"
+version = "0.5.1"
+license = "Apache-2.0"
+authors = ["Allen George <al...@gmail.com>"]
+readme = "README.md"
+documentation = "https://docs.rs/rift/"
+exclude = ["Makefile*"]
+keywords = ["thrift"]
+
--- End diff --
I was more referring to the legalese section. If its ASF code, then it should say ASF.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94346784
--- Diff: test/ThriftTest.thrift ---
@@ -67,7 +67,7 @@ typedef i64 UserId
struct Bonk
{
1: string message,
- 2: i32 type
+ 2: i32 bonkType
}
--- End diff --
Well, first, it is a keyword in Delphi too. Next, you cannot safely assume that anyone offering a Thrift API will test all languages implemented today if they work with the curent names you use in your IDL. And you cannot look into the future and foresee what language Apple or Google or whoever else comes around next week which may use "bonkType" as a keyword. So we have to deal with it.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1147
Hi guys - any comments on this PR? I think I've addressed all review comments.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by anatol <gi...@git.apache.org>.
Github user anatol commented on the issue:
https://github.com/apache/thrift/pull/1147
@allengeorge Hi Allen, I was intended to send only `.gitignore` comments related to `erl`. The other two comments were create a long time ago and for some reason github decided to publish it this time. But you've already resolved it. Sorry for the confusion. And thanks one more time for your work.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by anatol <gi...@git.apache.org>.
Github user anatol commented on the issue:
https://github.com/apache/thrift/pull/1147
This patch looks really good. Thank you @allengeorge for your work! Thrift team, is there anything left that prevents rust library from merging?
Is there a such status as 'thrift library maintainer'. It would be great if @allengeorge keep maintaining rust library and `thrift` cargo.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by anatol <gi...@git.apache.org>.
Github user anatol commented on the issue:
https://github.com/apache/thrift/pull/1147
* Non-Rust changes (like C# test fix) should be extracted into separate change.
* Could you please run 'rustfmt' tool over rust sources to bring the sources to common codestyle?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1147
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1147: THRIFT-2945 Add Rust support
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1147#discussion_r94354976
--- Diff: lib/rs/Cargo.toml ---
@@ -0,0 +1,17 @@
+[package]
+name = "rift"
+description = "Rust Thrift library"
+version = "0.5.1"
+license = "Apache-2.0"
+authors = ["Allen George <al...@gmail.com>"]
+readme = "README.md"
+documentation = "https://docs.rs/rift/"
+exclude = ["Makefile*"]
+keywords = ["thrift"]
+
--- End diff --
No problem.
1. Should the authors be "Thrift developers"?
2. Isn't the license OK?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1147: THRIFT-2945 Add Rust support
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1147
The usualities regarding tests are as follows:
* the ThriftTest suite tests should be placed under /test/<language>
* any other tests that do not belong to the Test Suite should be placed under /lib/<language>/test
Don't worry, common mistake. A lot of people did it wrong at least once, myself included.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---