You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by cdwijayarathna <gi...@git.apache.org> on 2014/08/15 19:59:04 UTC
[GitHub] thrift pull request: THRIFT-847 Test Framework harmonization acros...
GitHub user cdwijayarathna opened a pull request:
https://github.com/apache/thrift/pull/190
THRIFT-847 Test Framework harmonization across all languages
Haskell tests added to test.py and test.sh
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cdwijayarathna/thrift THRIFT-847
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/190.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 #190
----
commit 4f96910047ecf7c18a40b74191cd7b501508fc55
Author: cdwijayarathna <cd...@gmail.com>
Date: 2014-08-15T16:48:30Z
THRIFT-847 Test Framework harmonization across all languages
----
---
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: THRIFT-847 Test Framework harmonization acros...
Posted by bufferoverflow <gi...@git.apache.org>.
Github user bufferoverflow commented on the pull request:
https://github.com/apache/thrift/pull/190#issuecomment-52357551
The changes within .travis.yml and contrib/installDependencies.sh are not required.
you can do this:
{noformat}
diff --git a/lib/hs/Thrift.cabal b/lib/hs/Thrift.cabal
index f847663..eb60bb5 100755
--- a/lib/hs/Thrift.cabal
+++ b/lib/hs/Thrift.cabal
@@ -36,7 +36,7 @@ Library
Hs-Source-Dirs:
src
Build-Depends:
- base >= 4, base < 5, containers, network, ghc-prim, attoparsec, binary, bytestring >= 0.10, hashable, HTTP, text,
+ base >= 4, base < 5, containers, network, ghc-prim, attoparsec, binary, bytestring >= 0.10, hashable, HTTP, text,
Exposed-Modules:
Thrift,
Thrift.Arbitraries
{noformat}
test/tests.json is committed
---
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: THRIFT-847 Test Framework harmonization acros...
Posted by zilberstein <gi...@git.apache.org>.
Github user zilberstein commented on a diff in the pull request:
https://github.com/apache/thrift/pull/190#discussion_r16317049
--- Diff: test/hs/TestClient.hs ---
@@ -197,24 +198,24 @@ main = do
Binary -> runClient $ BinaryProtocol handle
Compact -> runClient $ CompactProtocol handle
JSON -> runClient $ JSONProtocol handle
- replicateM_ testLoops client
+ replicateM_ testLoops client
putStrLn "COMPLETED SUCCESSFULLY"
parseFlags :: [String] -> Options -> Maybe Options
+parseFlags (flag : flags) opts = do
+ let pieces = splitOn "=" flag
+ case pieces of
--- End diff --
1) The pattern match case on line 204/217 should be integrated into the `case` statement here.
2) You match a couple of strings, but a non-exhaustive pattern match error will be thrown if none of these strings match. You should add a default case with the wildcard operator (`_`) that evaluated to `Nothing` so that if you enter an unsupported flag it will print the help message
---
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: THRIFT-847 Test Framework harmonization acros...
Posted by cdwijayarathna <gi...@git.apache.org>.
Github user cdwijayarathna commented on a diff in the pull request:
https://github.com/apache/thrift/pull/190#discussion_r16325997
--- Diff: test/hs/TestClient.hs ---
@@ -197,24 +198,24 @@ main = do
Binary -> runClient $ BinaryProtocol handle
Compact -> runClient $ CompactProtocol handle
JSON -> runClient $ JSONProtocol handle
- replicateM_ testLoops client
+ replicateM_ testLoops client
putStrLn "COMPLETED SUCCESSFULLY"
parseFlags :: [String] -> Options -> Maybe Options
+parseFlags (flag : flags) opts = do
+ let pieces = splitOn "=" flag
+ case pieces of
--- End diff --
https://github.com/apache/thrift/pull/193
---
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: THRIFT-847 Test Framework harmonization acros...
Posted by zilberstein <gi...@git.apache.org>.
Github user zilberstein commented on a diff in the pull request:
https://github.com/apache/thrift/pull/190#discussion_r16317067
--- Diff: test/hs/TestClient.hs ---
@@ -197,24 +198,24 @@ main = do
Binary -> runClient $ BinaryProtocol handle
Compact -> runClient $ CompactProtocol handle
JSON -> runClient $ JSONProtocol handle
- replicateM_ testLoops client
+ replicateM_ testLoops client
putStrLn "COMPLETED SUCCESSFULLY"
parseFlags :: [String] -> Options -> Maybe Options
+parseFlags (flag : flags) opts = do
+ let pieces = splitOn "=" flag
+ case pieces of
+ "--port" : arg : _ -> parseFlags flags opts{ port = read arg }
+ "--domain-socket" : arg : _ -> parseFlags flags opts{ domainSocket = read arg }
+ "--host" : arg : _ -> parseFlags flags opts{ host = arg }
+ "--transport" : arg : _ -> parseFlags flags opts{ transport = arg }
+ "--protocol" : arg : _ -> parseFlags flags opts{ protocol = getProtocol arg }
+ "--h" : _ -> Nothing
+ "--help" : _ -> Nothing
+ "--ssl" : _ -> parseFlags flags opts{ ssl = True }
+ "--processor-events" : _ -> parseFlags flags opts
--- End diff --
ie, after this line
_ -> Nothing
---
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: THRIFT-847 Test Framework harmonization acros...
Posted by zilberstein <gi...@git.apache.org>.
Github user zilberstein commented on a diff in the pull request:
https://github.com/apache/thrift/pull/190#discussion_r16325863
--- Diff: test/hs/TestClient.hs ---
@@ -197,24 +198,24 @@ main = do
Binary -> runClient $ BinaryProtocol handle
Compact -> runClient $ CompactProtocol handle
JSON -> runClient $ JSONProtocol handle
- replicateM_ testLoops client
+ replicateM_ testLoops client
putStrLn "COMPLETED SUCCESSFULLY"
parseFlags :: [String] -> Options -> Maybe Options
+parseFlags (flag : flags) opts = do
+ let pieces = splitOn "=" flag
+ case pieces of
--- End diff --
see https://gist.github.com/zilberstein/52a4e04788810e0f1ce4
---
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: THRIFT-847 Test Framework harmonization acros...
Posted by cdwijayarathna <gi...@git.apache.org>.
Github user cdwijayarathna commented on the pull request:
https://github.com/apache/thrift/pull/190#issuecomment-52356290
@bufferoverflow Added changes suggested by @zilberstein in https://github.com/cdwijayarathna/thrift/commit/33fe8f6da989181060d4c502a27426b7d57ff046 , should I create new PR with a single commit?
---
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: THRIFT-847 Test Framework harmonization acros...
Posted by bufferoverflow <gi...@git.apache.org>.
Github user bufferoverflow commented on the pull request:
https://github.com/apache/thrift/pull/190#issuecomment-52359230
You also might start using --port=${THRIFT_TEST_PORT}
---
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: THRIFT-847 Test Framework harmonization acros...
Posted by cdwijayarathna <gi...@git.apache.org>.
Github user cdwijayarathna commented on a diff in the pull request:
https://github.com/apache/thrift/pull/190#discussion_r16324539
--- Diff: test/hs/TestClient.hs ---
@@ -197,24 +198,24 @@ main = do
Binary -> runClient $ BinaryProtocol handle
Compact -> runClient $ CompactProtocol handle
JSON -> runClient $ JSONProtocol handle
- replicateM_ testLoops client
+ replicateM_ testLoops client
putStrLn "COMPLETED SUCCESSFULLY"
parseFlags :: [String] -> Options -> Maybe Options
+parseFlags (flag : flags) opts = do
+ let pieces = splitOn "=" flag
+ case pieces of
--- End diff --
How can I integrate two cases?
---
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: THRIFT-847 Test Framework harmonization acros...
Posted by cdwijayarathna <gi...@git.apache.org>.
Github user cdwijayarathna commented on the pull request:
https://github.com/apache/thrift/pull/190#issuecomment-52358462
@bufferoverflow Added suggested changes and committed. Is there anything else I need to do for 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: THRIFT-847 Test Framework harmonization acros...
Posted by cdwijayarathna <gi...@git.apache.org>.
Github user cdwijayarathna commented on the pull request:
https://github.com/apache/thrift/pull/190#issuecomment-52359274
I removed trailing white spaces in files I edited, do I need to undo them?
---
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: THRIFT-847 Test Framework harmonization acros...
Posted by bufferoverflow <gi...@git.apache.org>.
Github user bufferoverflow commented on the pull request:
https://github.com/apache/thrift/pull/190#issuecomment-52358999
please fix all the white space issues
e.g contrib/installDependencies.sh and many other areas had no changes
---
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: THRIFT-847 Test Framework harmonization acros...
Posted by zilberstein <gi...@git.apache.org>.
Github user zilberstein commented on the pull request:
https://github.com/apache/thrift/pull/190#issuecomment-52356546
@cdwijayarathna, you can squash your commits and then force push to the branch
---
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: THRIFT-847 Test Framework harmonization acros...
Posted by zilberstein <gi...@git.apache.org>.
Github user zilberstein commented on a diff in the pull request:
https://github.com/apache/thrift/pull/190#discussion_r16316474
--- Diff: test/hs/TestClient.hs ---
@@ -197,24 +198,24 @@ main = do
Binary -> runClient $ BinaryProtocol handle
Compact -> runClient $ CompactProtocol handle
JSON -> runClient $ JSONProtocol handle
- replicateM_ testLoops client
+ replicateM_ testLoops client
putStrLn "COMPLETED SUCCESSFULLY"
parseFlags :: [String] -> Options -> Maybe Options
+parseFlags (flag : flags) opts = do
+ let pieces = splitOn "=" flag
+ case pieces of
--- End diff --
This pattern match is not exhaustive. Also add the `-n` case here
---
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: THRIFT-847 Test Framework harmonization acros...
Posted by cdwijayarathna <gi...@git.apache.org>.
Github user cdwijayarathna closed the pull request at:
https://github.com/apache/thrift/pull/190
---
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: THRIFT-847 Test Framework harmonization acros...
Posted by bufferoverflow <gi...@git.apache.org>.
Github user bufferoverflow commented on the pull request:
https://github.com/apache/thrift/pull/190#issuecomment-52360323
sorry, I did not recognized that. Perfect!
committed
---
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: THRIFT-847 Test Framework harmonization acros...
Posted by cdwijayarathna <gi...@git.apache.org>.
Github user cdwijayarathna commented on a diff in the pull request:
https://github.com/apache/thrift/pull/190#discussion_r16316696
--- Diff: test/hs/TestClient.hs ---
@@ -197,24 +198,24 @@ main = do
Binary -> runClient $ BinaryProtocol handle
Compact -> runClient $ CompactProtocol handle
JSON -> runClient $ JSONProtocol handle
- replicateM_ testLoops client
+ replicateM_ testLoops client
putStrLn "COMPLETED SUCCESSFULLY"
parseFlags :: [String] -> Options -> Maybe Options
+parseFlags (flag : flags) opts = do
+ let pieces = splitOn "=" flag
+ case pieces of
--- End diff --
Sorry, I didn't understand what you suggested,
---
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.
---