-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BEAST_DEFINE_TESTSUITE_MANUAL(ProtocolJson,protocol,ripple); #1972
Conversation
Current coverage is 67.06% (diff: 0.00%)@@ develop #1972 diff @@
==========================================
Files 686 686
Lines 49340 49388 +48
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 33132 33121 -11
- Misses 16208 16267 +59
Partials 0 0
|
Yeah, the schema/code could be improved but I think this would be a very useful feature |
Json::Value all(Json::arrayValue); | ||
for (auto const& pair : knownCodeToField) | ||
{ | ||
if (pair.second->isBinary()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of makes the 'isBinary' output pointless ...
The reason it was there in the first place was that that there's some Hash256 fields like index
and hash
that are useful to declare, such that any Json -> dehydrated class works simply. There's also other Amount fields like "taker_gets_funded" etc in offers json.
ff9d822
to
eeff63b
Compare
Jenkins Build SummaryBuilt from this commit Built at 20190618 - 00:20:01 Test Results
|
eeff63b
to
6d57472
Compare
6d57472
to
0ae30be
Compare
Thank you for your submission. It will be reviewed soon and submitted for processing in CI. |
6 similar comments
Thank you for your submission. It will be reviewed soon and submitted for processing in CI. |
Thank you for your submission. It will be reviewed soon and submitted for processing in CI. |
Thank you for your submission. It will be reviewed soon and submitted for processing in CI. |
Thank you for your submission. It will be reviewed soon and submitted for processing in CI. |
Thank you for your submission. It will be reviewed soon and submitted for processing in CI. |
Thank you for your submission. It will be reviewed soon and submitted for processing in CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the commenting out of several lovely checks here so I can't give a thumbs up.
More broadly, while I am supportive of this, I think it would probably be better if it was a standalone tool that parsed the source code. 🤔
Parse the C++ code with clang/regexes or sth? That would be a pain in the
ass to dev/maintain. Or you have sth else in mind? If you could do it in a
separate tool rather than abusing the unittest machinery that would be
cool. Were you thinking something like the manifest signing tool that
imports the xrpl/core (or whatever) lib?
Yeah, I don’t like commenting out checks, but I just kept it rebased so I
could spit out some JSON now and then. It’s been quite useful. Some people
asked me to do a bit of updating on ripple-lib-java a while back, and I was
getting a headache eyeballing all the changes so I added some tests to
check the JSON matches all the static Java declarations.
|
parsed the source code.
Aaaaah. I see.
|
Actually, now that I remember, (iirc) there are some things that were only
avail (easily) at runtime. I first messed with something like this using
gdb and a python plugin back in the day. Looked into clang api and was all
ugh, no time for this, then used test suite.
|
I would like to merge this but the commenting of several For example:
And:
|
Right, moving the source of truth to something other than hard to parse C++
sounds good to me :)
This was just a pragmatic solution at the time.
Not offended if y’all close the PR
|
@nbougalis |
|
Just leaving this quick hack here as a starting point for you C++ gurus to work off/consider. This is a manually run test that dumps the protocol definitions in json, which I used to update ripple-lib-java for omnigate.com.
It's too error-prone to do manually so I hacked this in to spit out json, and then loaded the json inside a java unit test and made sure all the Java definitions (using native enums/classes etc) were in sync.
It would be quite a useful command/feature to have, to keep the various ripple-lib-* up to date. It might be more nicely exposed as an RPC command.