Skip to content
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

Continue documenting grpc.proto for API reference site [#7] #6068

Merged
merged 44 commits into from
Feb 28, 2022

Conversation

ghubstan
Copy link
Contributor

This completes a first pass of grpc.proto documentation, after some .proto field name and field type changes were made in previous PRs. Comments have not been added to pb.proto defs referenced by the API site, such as all the *PaymentAccount messages. These may be commented later, but providing CLI, Java and Python code examples for the API reference doc is more important at this time.

While still a work in progress, the most recent version of the slate based API Reference document could be checked for correctness, missing detail, and to see if more .proto rpc method and/or field types or names need to be changed.

Based on branch 6-disambiguate-api-pct-field-names, PR #6065.

The new console output formatting (tbl) api is working, and the
deprecated classes just add maintenance work.

Update affected test cases.
Much less ambiguous field value at cost of breaking backward compat in API.
Quite a bit of refactoring.  A lot of number-string formatting was removed.
Rename double marketPriceMargin -> marketPriceMarginPct.
Rename double buyerSecurityDeposit -> buyerSecurityDepositPct.
@ghubstan ghubstan changed the title [WIP] Continue documenting grpc.proto for API reference site [WIP] Continue documenting grpc.proto for API reference site [#7] Feb 21, 2022
@ghubstan
Copy link
Contributor Author

I think the markdown->html transformation by the slate engine works pretty well, but there are two problems so far.

  1. When the browser is showing the lowest parts of the page, the nav links default to the last header:
    TOC Links collapses if not enough content to fill the screen slatedocs/slate#1440

  2. Reloading the html page can result in navigation to a different internal link. This seems related to the presence of code examples in the generated markdown. I'll learn more as I start including more code snippets.

This was overlooked in tests, but client might use a bad offer-id param.
@ghubstan ghubstan changed the title [WIP] Continue documenting grpc.proto for API reference site [#7] Continue documenting grpc.proto for API reference site [#7] Feb 25, 2022
@ghubstan ghubstan marked this pull request as ready for review February 26, 2022 00:04
ghubstan added a commit to ghubstan/bisq that referenced this pull request Feb 26, 2022
This protobuf definition and service stub has been in place since
the start of work on the API, but was never fully implmented, nor
intended to be included in the API beta & v1 releases.

Its presence added a useless section to the gRPC API Reference doc.

	https://ghubstan.github.io/slate

Based on branch `7-more-grpcproto-comments`,
PR bisq-network#6068.
The CLI and apitest cases always pass "0", but java & python clients
might pass an empty string. This change avoids number formatting
& scaling problems when clients pass an empty string in the price
parameter.
The client may have passed an empty string for the price parameter,
if only enabling or disabling the offer.  If so, validate with new
price = old price.
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK - Changes are looking fine 👍

@ripcurlx ripcurlx added this to the v1.8.4 milestone Feb 28, 2022
@ripcurlx ripcurlx merged commit d301b0d into bisq-network:master Feb 28, 2022
@ghubstan ghubstan deleted the 7-more-grpcproto-comments branch February 28, 2022 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants