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
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
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
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)
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
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
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
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
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
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
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
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
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
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
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
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,
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
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.
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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`
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.
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
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
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/
* any other tests that do not belong to the Test Suite should be placed
under
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:
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
39 matches
Mail list logo
